Re: [Qemu-devel] [PATCH 01/10] tcg: Split out swap_commutative as a subroutine

[Aurelien Jarno]

On Tue, Oct 02, 2012 at 11:32:21AM -0700, Richard Henderson wrote:
> Reduces code duplication and prefers
> 
>   movcond d, c1, c2, const, s
> to
>   movcond d, c1, c2, s, const
> 
> It also prefers
> 
>   add r, r, c
> over
>   add r, c, r
> 
> when both inputs are known constants.  This doesn't matter for true add, as
> we will fully constant fold that.  But it matters for a follow-on patch using
> this routine for add2 which may not be fully foldable.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/optimize.c | 56 ++++++++++++++++++++++++--------------------------------
>  1 file changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index 35532a1..5e0504a 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -382,6 +382,23 @@ static TCGArg do_constant_folding_cond(TCGOpcode op, 
> TCGArg x,
>      tcg_abort();
>  }
>  
> +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;
> +
> +    /* Prefer the constant in second argument, and then the form
> +       op a, a, b, which is better handled on non-RISC hosts. */
> +    if (sum > 0 || (sum == 0 && dest == a2)) {
> +        *p1 = a2;
> +        *p2 = a1;
> +        return true;
> +    }
> +    return false;
> +}
> +

Does this sum += and -= actually generates better code than the previous
one? It's not something obvious to read (fortunately there is the
comment for helping), so if it doesn't bring any optimization, it's
better to keep the previous form.

>  /* Propagate constants and copies, fold constant expressions. */
>  static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr,
>                                      TCGArg *args, TCGOpDef *tcg_op_defs)
> @@ -391,7 +408,6 @@ static TCGArg *tcg_constant_folding(TCGContext *s, 
> uint16_t *tcg_opc_ptr,
>      const TCGOpDef *def;
>      TCGArg *gen_args;
>      TCGArg tmp;
> -    TCGCond cond;
>  
>      /* Array VALS has an element for each temp.
>         If this temp holds a constant then its value is kept in VALS' element.
> @@ -434,52 +450,28 @@ static TCGArg *tcg_constant_folding(TCGContext *s, 
> uint16_t *tcg_opc_ptr,
>          CASE_OP_32_64(eqv):
>          CASE_OP_32_64(nand):
>          CASE_OP_32_64(nor):
> -            /* Prefer the constant in second argument, and then the form
> -               op a, a, b, which is better handled on non-RISC hosts. */
> -            if (temps[args[1]].state == TCG_TEMP_CONST || (args[0] == args[2]
> -                && temps[args[2]].state != TCG_TEMP_CONST)) {
> -                tmp = args[1];
> -                args[1] = args[2];
> -                args[2] = tmp;
> -            }
> +            swap_commutative(args[0], &args[1], &args[2]);
>              break;
>          CASE_OP_32_64(brcond):
> -            if (temps[args[0]].state == TCG_TEMP_CONST
> -                && temps[args[1]].state != TCG_TEMP_CONST) {
> -                tmp = args[0];
> -                args[0] = args[1];
> -                args[1] = tmp;
> +            if (swap_commutative(-1, &args[0], &args[1])) {
>                  args[2] = tcg_swap_cond(args[2]);
>              }
>              break;
>          CASE_OP_32_64(setcond):
> -            if (temps[args[1]].state == TCG_TEMP_CONST
> -                && temps[args[2]].state != TCG_TEMP_CONST) {
> -                tmp = args[1];
> -                args[1] = args[2];
> -                args[2] = tmp;
> +            if (swap_commutative(args[0], &args[1], &args[2])) {
>                  args[3] = tcg_swap_cond(args[3]);
>              }
>              break;
>          CASE_OP_32_64(movcond):
> -            cond = args[5];
> -            if (temps[args[1]].state == TCG_TEMP_CONST
> -                && temps[args[2]].state != TCG_TEMP_CONST) {
> -                tmp = args[1];
> -                args[1] = args[2];
> -                args[2] = tmp;
> -                cond = tcg_swap_cond(cond);
> +            if (swap_commutative(-1, &args[1], &args[2])) {
> +                args[5] = tcg_swap_cond(args[5]);
>              }
>              /* For movcond, we canonicalize the "false" input reg to match
>                 the destination reg so that the tcg backend can implement
>                 a "move if true" operation.  */
> -            if (args[0] == args[3]) {
> -                tmp = args[3];
> -                args[3] = args[4];
> -                args[4] = tmp;
> -                cond = tcg_invert_cond(cond);
> +            if (swap_commutative(args[0], &args[4], &args[3])) {
> +                args[5] = tcg_invert_cond(args[5]);
>              }
> -            args[5] = cond;
>          default:
>              break;
>          }

Reviewed-by: Aurelien Jarno <address@hidden>


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net