Patchwork [03/10] tcg: Swap commutative double-word comparisons

login
register
mail settings
Submitter Richard Henderson
Date Oct. 2, 2012, 6:32 p.m.
Message ID <1349202750-16815-4-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/188606/
State New
Headers show

Comments

Richard Henderson - Oct. 2, 2012, 6:32 p.m.
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/optimize.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
Aurelien Jarno - Oct. 9, 2012, 3:16 p.m.
On Tue, Oct 02, 2012 at 11:32:23AM -0700, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/optimize.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index 3539826..a713513 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -399,6 +399,22 @@ static bool swap_commutative(TCGArg dest, TCGArg *p1, TCGArg *p2)
>      return false;
>  }
>  
> +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;
> +    if (sum > 0) {
> +        TCGArg t;
> +        t = p1[0], p1[0] = p2[0], p2[0] = t;
> +        t = p1[1], p1[1] = p2[1], p2[1] = t;
> +        return true;
> +    }
> +    return false;
> +}
> +
>  /* 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)
> @@ -477,6 +493,16 @@ static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr,
>              swap_commutative(args[0], &args[2], &args[4]);
>              swap_commutative(args[1], &args[3], &args[5]);
>              break;
> +        case INDEX_op_brcond2_i32:
> +            if (swap_commutative2(&args[0], &args[2])) {
> +                args[4] = tcg_swap_cond(args[4]);
> +            }
> +            break;
> +        case INDEX_op_setcond2_i32:
> +            if (swap_commutative2(&args[1], &args[3])) {
> +                args[5] = tcg_swap_cond(args[5]);
> +            }
> +            break;
>          default:
>              break;
>          }

Same comment are for the swap_commutative() patch, otherwise:

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
Richard Henderson - Oct. 9, 2012, 3:31 p.m.
On 10/09/2012 08:16 AM, Aurelien Jarno wrote:
>> > +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;
>> > +    if (sum > 0) {
...
> Same comment are for the swap_commutative() patch, otherwise:

While I don't have an explicit test case for swap_commutative2 like
I do for swap_commutative, think about how many conditionals you'd
have to use to write this without using SUM:

  if (((temps[p1[0]].state == TCG_TEMP_CONST            // if both p1 are const
        && temps[p1[1]].state == TCG_TEMP_CONST
        && !(temps[p2[0]].state == TCG_TEMP_CONST       // ... and not both p2 are const
             && temps[p2[1]].state == TCG_TEMP_CONST))
      || ((temps[p1[0]].state == TCG_TEMP_CONST         // if either p1 are const
           || temps[p1[1]].state == TCG_TEMP_CONST)
          && !temps[p2[0]].state == TCG_TEMP_CONST      // ... and neither p2 are const
          && !temps[p2[1]].state == TCG_TEMP_CONST))

I don't see how that can possibly be easier to understand.


r~
Aurelien Jarno - Oct. 9, 2012, 3:48 p.m.
On Tue, Oct 09, 2012 at 08:31:47AM -0700, Richard Henderson wrote:
> On 10/09/2012 08:16 AM, Aurelien Jarno wrote:
> >> > +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;
> >> > +    if (sum > 0) {
> ...
> > Same comment are for the swap_commutative() patch, otherwise:
> 
> While I don't have an explicit test case for swap_commutative2 like
> I do for swap_commutative, think about how many conditionals you'd
> have to use to write this without using SUM:
> 
>   if (((temps[p1[0]].state == TCG_TEMP_CONST            // if both p1 are const
>         && temps[p1[1]].state == TCG_TEMP_CONST
>         && !(temps[p2[0]].state == TCG_TEMP_CONST       // ... and not both p2 are const
>              && temps[p2[1]].state == TCG_TEMP_CONST))
>       || ((temps[p1[0]].state == TCG_TEMP_CONST         // if either p1 are const
>            || temps[p1[1]].state == TCG_TEMP_CONST)
>           && !temps[p2[0]].state == TCG_TEMP_CONST      // ... and neither p2 are const
>           && !temps[p2[1]].state == TCG_TEMP_CONST))
> 
> I don't see how that can possibly be easier to understand.
> 
> 

For that one I agree.

Patch

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 3539826..a713513 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -399,6 +399,22 @@  static bool swap_commutative(TCGArg dest, TCGArg *p1, TCGArg *p2)
     return false;
 }
 
+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;
+    if (sum > 0) {
+        TCGArg t;
+        t = p1[0], p1[0] = p2[0], p2[0] = t;
+        t = p1[1], p1[1] = p2[1], p2[1] = t;
+        return true;
+    }
+    return false;
+}
+
 /* 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)
@@ -477,6 +493,16 @@  static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr,
             swap_commutative(args[0], &args[2], &args[4]);
             swap_commutative(args[1], &args[3], &args[5]);
             break;
+        case INDEX_op_brcond2_i32:
+            if (swap_commutative2(&args[0], &args[2])) {
+                args[4] = tcg_swap_cond(args[4]);
+            }
+            break;
+        case INDEX_op_setcond2_i32:
+            if (swap_commutative2(&args[1], &args[3])) {
+                args[5] = tcg_swap_cond(args[5]);
+            }
+            break;
         default:
             break;
         }