Re: [Qemu-devel] [PATCH for-2.5 02/10] tcg/optimize: add temp_is_const and temp_is_copy functions

[Aurelien Jarno]

On 2015-07-29 17:01, Alex Bennée wrote:
> 
> Aurelien Jarno <address@hidden> writes:
> 
> > Add two accessor functions temp_is_const and temp_is_copy, to make the
> > code more readable and make code change easier.
> >
> > Cc: Richard Henderson <address@hidden>
> > Signed-off-by: Aurelien Jarno <address@hidden>
> > ---
> >  tcg/optimize.c | 131 
> > ++++++++++++++++++++++++++-------------------------------
> >  1 file changed, 60 insertions(+), 71 deletions(-)
> >
> > diff --git a/tcg/optimize.c b/tcg/optimize.c
> > index 20e24b3..d2b63a4 100644
> > --- a/tcg/optimize.c
> > +++ b/tcg/optimize.c
> > @@ -52,11 +52,21 @@ struct tcg_temp_info {
> >  static struct tcg_temp_info temps[TCG_MAX_TEMPS];
> >  static TCGTempSet temps_used;
> >  
> > +static inline bool temp_is_const(TCGArg arg)
> > +{
> > +    return temps[arg].state == TCG_TEMP_CONST;
> > +}
> > +
> > +static inline bool temp_is_copy(TCGArg arg)
> > +{
> > +    return temps[arg].state == TCG_TEMP_COPY;
> > +}
> > +
> >  /* Reset TEMP's state to TCG_TEMP_UNDEF.  If TEMP only had one copy, remove
> >     the copy flag from the left temp.  */
> >  static void reset_temp(TCGArg temp)
> >  {
> > -    if (temps[temp].state == TCG_TEMP_COPY) {
> > +    if (temp_is_copy(temp)) {
> >          if (temps[temp].prev_copy == temps[temp].next_copy) {
> >              temps[temps[temp].next_copy].state = TCG_TEMP_UNDEF;
> >          } else {
> > @@ -186,8 +196,7 @@ static bool temps_are_copies(TCGArg arg1, TCGArg arg2)
> >          return true;
> >      }
> >  
> > -    if (temps[arg1].state != TCG_TEMP_COPY
> > -        || temps[arg2].state != TCG_TEMP_COPY) {
> > +    if (!temp_is_copy(arg1) || !temp_is_copy(arg2)) {
> >          return false;
> >      }
> >  
> > @@ -230,7 +239,7 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, 
> > TCGArg *args,
> >          return;
> >      }
> >  
> > -    if (temps[src].state == TCG_TEMP_CONST) {
> > +    if (temp_is_const(src)) {
> >          tcg_opt_gen_movi(s, op, args, dst, temps[src].val);
> >          return;
> >      }
> > @@ -248,10 +257,10 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, 
> > TCGArg *args,
> >      }
> >      temps[dst].mask = mask;
> >  
> > -    assert(temps[src].state != TCG_TEMP_CONST);
> > +    assert(!temp_is_const(src));
> >  
> >      if (s->temps[src].type == s->temps[dst].type) {
> > -        if (temps[src].state != TCG_TEMP_COPY) {
> > +        if (!temp_is_copy(src)) {
> >              temps[src].state = TCG_TEMP_COPY;
> >              temps[src].next_copy = src;
> >              temps[src].prev_copy = src;
> > @@ -488,7 +497,7 @@ static bool do_constant_folding_cond_eq(TCGCond c)
> >  static TCGArg do_constant_folding_cond(TCGOpcode op, TCGArg x,
> >                                         TCGArg y, TCGCond c)
> >  {
> > -    if (temps[x].state == TCG_TEMP_CONST && temps[y].state == 
> > TCG_TEMP_CONST) {
> > +    if (temp_is_const(x) && temp_is_const(y)) {
> >          switch (op_bits(op)) {
> >          case 32:
> >              return do_constant_folding_cond_32(temps[x].val, temps[y].val, 
> > c);
> > @@ -499,7 +508,7 @@ static TCGArg do_constant_folding_cond(TCGOpcode op, 
> > TCGArg x,
> >          }
> >      } else if (temps_are_copies(x, y)) {
> >          return do_constant_folding_cond_eq(c);
> > -    } else if (temps[y].state == TCG_TEMP_CONST && temps[y].val == 0) {
> > +    } else if (temp_is_const(y) && temps[y].val == 0) {
> >          switch (c) {
> >          case TCG_COND_LTU:
> >              return 0;
> > @@ -520,12 +529,10 @@ static TCGArg do_constant_folding_cond2(TCGArg *p1, 
> > TCGArg *p2, TCGCond c)
> >      TCGArg al = p1[0], ah = p1[1];
> >      TCGArg bl = p2[0], bh = p2[1];
> >  
> > -    if (temps[bl].state == TCG_TEMP_CONST
> > -        && temps[bh].state == TCG_TEMP_CONST) {
> > +    if (temp_is_const(bl) && temp_is_const(bh)) {
> >          uint64_t b = ((uint64_t)temps[bh].val << 32) | 
> > (uint32_t)temps[bl].val;
> >  
> > -        if (temps[al].state == TCG_TEMP_CONST
> > -            && temps[ah].state == TCG_TEMP_CONST) {
> > +        if (temp_is_const(al) && temp_is_const(ah)) {
> >              uint64_t a;
> >              a = ((uint64_t)temps[ah].val << 32) | (uint32_t)temps[al].val;
> >              return do_constant_folding_cond_64(a, b, c);
> > @@ -551,8 +558,8 @@ static bool swap_commutative(TCGArg dest, TCGArg *p1, 
> > TCGArg *p2)
> >  {
> >      TCGArg a1 = *p1, a2 = *p2;
> >      int sum = 0;
> > -    sum += temps[a1].state == TCG_TEMP_CONST;
> > -    sum -= temps[a2].state == TCG_TEMP_CONST;
> > +    sum += temp_is_const(a1);
> > +    sum -= temp_is_const(a2);
> >  
> >      /* Prefer the constant in second argument, and then the form
> >         op a, a, b, which is better handled on non-RISC hosts. */
> > @@ -567,10 +574,10 @@ static bool swap_commutative(TCGArg dest, TCGArg *p1, 
> > TCGArg *p2)
> >  static bool swap_commutative2(TCGArg *p1, TCGArg *p2)
> >  {
> >      int sum = 0;
> > -    sum += temps[p1[0]].state == TCG_TEMP_CONST;
> > -    sum += temps[p1[1]].state == TCG_TEMP_CONST;
> > -    sum -= temps[p2[0]].state == TCG_TEMP_CONST;
> > -    sum -= temps[p2[1]].state == TCG_TEMP_CONST;
> > +    sum += temp_is_const(p1[0]);
> > +    sum += temp_is_const(p1[1]);
> > +    sum -= temp_is_const(p2[0]);
> > +    sum -= temp_is_const(p2[1]);
> >      if (sum > 0) {
> >          TCGArg t;
> >          t = p1[0], p1[0] = p2[0], p2[0] = t;
> > @@ -620,7 +627,7 @@ void tcg_optimize(TCGContext *s)
> >  
> >          /* Do copy propagation */
> >          for (i = nb_oargs; i < nb_oargs + nb_iargs; i++) {
> > -            if (temps[args[i]].state == TCG_TEMP_COPY) {
> > +            if (temp_is_copy(args[i])) {
> >                  args[i] = find_better_copy(s, args[i]);
> >              }
> >          }
> > @@ -690,8 +697,7 @@ void tcg_optimize(TCGContext *s)
> >          CASE_OP_32_64(sar):
> >          CASE_OP_32_64(rotl):
> >          CASE_OP_32_64(rotr):
> > -            if (temps[args[1]].state == TCG_TEMP_CONST
> > -                && temps[args[1]].val == 0) {
> > +            if (temp_is_const(args[1]) && temps[args[1]].val == 0) {
> >                  tcg_opt_gen_movi(s, op, args, args[0], 0);
> >                  continue;
> >              }
> > @@ -701,7 +707,7 @@ void tcg_optimize(TCGContext *s)
> >                  TCGOpcode neg_op;
> >                  bool have_neg;
> >  
> > -                if (temps[args[2]].state == TCG_TEMP_CONST) {
> > +                if (temp_is_const(args[2])) {
> >                      /* Proceed with possible constant folding. */
> >                      break;
> >                  }
> > @@ -715,8 +721,7 @@ void tcg_optimize(TCGContext *s)
> >                  if (!have_neg) {
> >                      break;
> >                  }
> > -                if (temps[args[1]].state == TCG_TEMP_CONST
> > -                    && temps[args[1]].val == 0) {
> > +                if (temp_is_const(args[1]) && temps[args[1]].val ==
> > 0) {
> 
> This makes me wonder if we should have:
> 
>   temp_is_const_val(arg, val)
> 
> to wrap up these tests even more neatly

That's something that can be added, but in that case we need both
temp_is_const and temp_is_const_val as we sometimes need to test if
a temp is a constant without necessarily checking for a particular
value.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net