[Valgrind-developers] [valgrind/grail] 'grail' fixes for arm32:

[Julian S. <se...@so...>]

https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=effd32dd0e79a3100e767c6b76a1635af0bf1002

commit effd32dd0e79a3100e767c6b76a1635af0bf1002
Author: Julian Seward <js...@ac...>
Date:   Wed Nov 27 06:37:42 2019 +0100

    'grail' fixes for arm32:
    
    * priv/guest_generic_bb_to_IR.c expr_is_guardable(), stmt_is_guardable():
      add some missing cases
    
    * do_minimal_initial_iropt_BB: add comment (no functional change)
    
    * priv/host_arm_isel.c iselCondCode_wrk(): handle And1 and Or1, the
      not-particularly-optimal way

Diff:
---
 VEX/priv/guest_generic_bb_to_IR.c |  4 ++++
 VEX/priv/host_arm_isel.c          | 24 ++++++++++++++++++++++++
 VEX/priv/ir_opt.c                 |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/VEX/priv/guest_generic_bb_to_IR.c b/VEX/priv/guest_generic_bb_to_IR.c
index 677cfca..6a1d4dc 100644
--- a/VEX/priv/guest_generic_bb_to_IR.c
+++ b/VEX/priv/guest_generic_bb_to_IR.c
@@ -426,6 +426,7 @@ static Bool expr_is_guardable ( const IRExpr* e )
       case Iex_CCall:
       case Iex_Get:
       case Iex_Const:
+      case Iex_RdTmp:
          return True;
       default:
          vex_printf("\n"); ppIRExpr(e); vex_printf("\n");
@@ -446,6 +447,7 @@ static Bool stmt_is_guardable ( const IRStmt* st )
 {
    switch (st->tag) {
       // These are easily guarded.
+      case Ist_NoOp:
       case Ist_IMark:
       case Ist_Put:
          return True;
@@ -458,6 +460,7 @@ static Bool stmt_is_guardable ( const IRStmt* st )
       // These could be guarded, with some effort, if really needed, but
       // currently aren't guardable.
       case Ist_Store:
+      case Ist_StoreG:
       case Ist_Exit:
          return False;
       // This is probably guardable, but it depends on the RHS of the
@@ -492,6 +495,7 @@ static void add_guarded_stmt_to_end_of ( /*MOD*/IRSB* bb,
                                          /*IN*/ IRStmt* st, IRTemp guard )
 {
    switch (st->tag) {
+      case Ist_NoOp:
       case Ist_IMark:
       case Ist_WrTmp:
          addStmtToIRSB(bb, st);
diff --git a/VEX/priv/host_arm_isel.c b/VEX/priv/host_arm_isel.c
index 510336b..acbd39a 100644
--- a/VEX/priv/host_arm_isel.c
+++ b/VEX/priv/host_arm_isel.c
@@ -1293,6 +1293,30 @@ static ARMCondCode iselCondCode_wrk ( ISelEnv* env, IRExpr* e )
       return e->Iex.Const.con->Ico.U1 ? ARMcc_EQ : ARMcc_NE;
    }
 
+   /* --- And1(x,y), Or1(x,y) --- */
+   /* FIXME: We could (and probably should) do a lot better here, by using the
+      iselCondCode_C/_R scheme used in the amd64 insn selector. */
+   if (e->tag == Iex_Binop
+       && (e->Iex.Binop.op == Iop_And1 || e->Iex.Binop.op == Iop_Or1)) {
+      HReg x_as_32 = newVRegI(env);
+      ARMCondCode cc_x = iselCondCode(env, e->Iex.Binop.arg1);
+      addInstr(env, ARMInstr_Mov(x_as_32, ARMRI84_I84(0,0)));
+      addInstr(env, ARMInstr_CMov(cc_x, x_as_32, ARMRI84_I84(1,0)));
+
+      HReg y_as_32 = newVRegI(env);
+      ARMCondCode cc_y = iselCondCode(env, e->Iex.Binop.arg2);
+      addInstr(env, ARMInstr_Mov(y_as_32, ARMRI84_I84(0,0)));
+      addInstr(env, ARMInstr_CMov(cc_y, y_as_32, ARMRI84_I84(1,0)));
+
+      HReg tmp = newVRegI(env);
+      ARMAluOp aop = e->Iex.Binop.op == Iop_And1 ? ARMalu_AND : ARMalu_OR;
+      addInstr(env, ARMInstr_Alu(aop, tmp, x_as_32, ARMRI84_R(y_as_32)));
+
+      ARMRI84* one  = ARMRI84_I84(1,0);
+      addInstr(env, ARMInstr_CmpOrTst(False/*test*/, tmp, one));
+      return ARMcc_NE;
+   }
+
    // JRS 2013-Jan-03: this seems completely nonsensical
    /* --- CasCmpEQ* --- */
    /* Ist_Cas has a dummy argument to compare with, so comparison is
diff --git a/VEX/priv/ir_opt.c b/VEX/priv/ir_opt.c
index d3bc989..7f39924 100644
--- a/VEX/priv/ir_opt.c
+++ b/VEX/priv/ir_opt.c
@@ -6783,6 +6783,8 @@ IRSB* do_minimal_initial_iropt_BB(IRSB* bb0) {
    redundant_get_removal_BB ( bb );
 
    // Do minimal constant prop: copy prop and constant prop only.  No folding.
+   // JRS FIXME 2019Nov25: this is too weak to be effective on arm32.  For that,
+   // specifying doFolding=True makes a huge difference.
    bb = cprop_BB_WRK ( bb, /*mustRetainNoOps=*/True,
                            /*doFolding=*/False );