[PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

[Maxim Kuvyrkov-2]

Hello,

The attached patch fixes PR37053
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37053#c5).

The problem was introduced by patch for PR28690
(http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00268.html, rtlanal.c
hunk); this hunk causes ICE on m68k (and, possibly, other) architectures.

Analysis:

Let's take instruction

(insn 309 2675 2677 36 postreload.c:446 (set (reg:SI 0 %d0)
         (plus:SI (mem/f:SI (reg:SI 8 %a0) [0 S4 A16])
             (reg:SI 0 %d0))) 132 {*addsi3_internal} (nil))

Before the patch reload was free to swap second (mem) and third (reg)
operand and match the pattern to the 4th alternative of

(define_insn "*addsi3_internal"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,d,a")
         (plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0,0")
                  (match_operand:SI 2 "general_src_operand"
"dIKLT,rJK,a,mSrIKLT,mSrIKLs")))]

After the patch, due to (mem) being a pointer, reload can no longer swap
the two operands and match the insn.

My first impulse was to remove the pointer tweak; but, well, if PowerPC
really benefits from it, let's make it a hook.

The attached patch does just that.  Bootstrapped on x86_64-linux-gnu and
cross-build to powerpc-linux-gnu (with no new warnings in logs); ok to
apply?


Thanks,

--
Maxim

2009-06-24  Maxim Kuvyrkov  <maxim@...>

        * doc/tm.texi (TARGET_COMMUTATIVE_OPERAND_PRECEDENCE): Document.
        * target.h (commutative_operand_precedence): New hook.
        * target-def.h (TARGET_COMMUTATIVE_OPERAND_PRECEDENCE): Define
        the default.
        * rtlanal.c (commutative_operand_precedence): Use the new hook.
        Move favoring of pointer objects to ...
        * config/rs6000/rs6000.c (rs6000_commutative_operand_precedence):
        ... here.  Implement hook.

Index: doc/tm.texi
===================================================================
--- doc/tm.texi (revision 148831)
+++ doc/tm.texi (working copy)
@@ -10592,6 +10592,18 @@ to analyze inner elements of @var{x} in
 passed along.
 @end deftypefn
 
+@deftypefn {Target Hook} int TARGET_COMMUTATIVE_OPERAND_PRECEDENCE (const_rtx @var{op}, int @var{value})
+This target hook returns a value indicating whether @var{OP}, an operand of
+a commutative operation, is preferred as the first or second operand.
+The higher the value, the stronger the preference for being the first operand.
+Negative values are used to indicate a preference for the first operand
+and positive values for the second operand.  Usually the hook will return
+@var{VALUE}, which is the default precedence for @var{OP}
+(see @file{rtlanal.c}:@code{commutative_operand_precedence()}), but sometimes
+backends may wish certain operands to appear at the right places within
+instructions.
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_SET_CURRENT_FUNCTION (tree @var{decl})
 The compiler invokes this hook whenever it changes its current function
 context (@code{cfun}).  You can define this function if
Index: target.h
===================================================================
--- target.h (revision 148831)
+++ target.h (working copy)
@@ -709,6 +709,10 @@ struct gcc_target
      FLAGS has the same meaning as in rtlanal.c: may_trap_p_1.  */
   int (* unspec_may_trap_p) (const_rtx x, unsigned flags);
 
+  /* Return a value indicating whether an operand of a commutative
+     operation is preferred as the first or second operand.  */
+  int (* commutative_operand_precedence) (const_rtx, int);
+
   /* Given a register, this hook should return a parallel of registers
      to represent where to find the register pieces.  Define this hook
      if the register and its mode are represented in Dwarf in
Index: target-def.h
===================================================================
--- target-def.h (revision 148831)
+++ target-def.h (working copy)
@@ -509,6 +509,7 @@
 #define TARGET_ALLOCATE_INITIAL_VALUE NULL
 
 #define TARGET_UNSPEC_MAY_TRAP_P default_unspec_may_trap_p
+#define TARGET_COMMUTATIVE_OPERAND_PRECEDENCE NULL
 
 #ifndef TARGET_SET_CURRENT_FUNCTION
 #define TARGET_SET_CURRENT_FUNCTION hook_void_tree
@@ -900,6 +901,7 @@
   TARGET_ADDRESS_COST, \
   TARGET_ALLOCATE_INITIAL_VALUE, \
   TARGET_UNSPEC_MAY_TRAP_P,                     \
+  TARGET_COMMUTATIVE_OPERAND_PRECEDENCE,        \
   TARGET_DWARF_REGISTER_SPAN,                   \
   TARGET_INIT_DWARF_REG_SIZES_EXTRA, \
   TARGET_FIXED_CONDITION_CODE_REGS, \
Index: rtlanal.c
===================================================================
--- rtlanal.c (revision 148831)
+++ rtlanal.c (working copy)
@@ -2905,62 +2905,78 @@ int
 commutative_operand_precedence (rtx op)
 {
   enum rtx_code code = GET_CODE (op);
+  int value;
   
   /* Constants always come the second operand.  Prefer "nice" constants.  */
   if (code == CONST_INT)
-    return -8;
-  if (code == CONST_DOUBLE)
-    return -7;
-  if (code == CONST_FIXED)
-    return -7;
-  op = avoid_constant_pool_reference (op);
-  code = GET_CODE (op);
-
-  switch (GET_RTX_CLASS (code))
-    {
-    case RTX_CONST_OBJ:
-      if (code == CONST_INT)
-        return -6;
-      if (code == CONST_DOUBLE)
-        return -5;
-      if (code == CONST_FIXED)
-        return -5;
-      return -4;
-
-    case RTX_EXTRA:
-      /* SUBREGs of objects should come second.  */
-      if (code == SUBREG && OBJECT_P (SUBREG_REG (op)))
-        return -3;
-      return 0;
+    value = -8;
+  else if (code == CONST_DOUBLE)
+    value = -7;
+  else if (code == CONST_FIXED)
+    value = -7;
+  else
+    {
+      op = avoid_constant_pool_reference (op);
+      code = GET_CODE (op);
+
+      switch (GET_RTX_CLASS (code))
+ {
+ case RTX_CONST_OBJ:
+  if (code == CONST_INT)
+    value = -6;
+  else if (code == CONST_DOUBLE)
+    value = -5;
+  else if (code == CONST_FIXED)
+    value = -5;
+  else
+    value = -4;
+  break;
+
+ case RTX_EXTRA:
+  /* SUBREGs of objects should come second.  */
+  if (code == SUBREG && OBJECT_P (SUBREG_REG (op)))
+    value = -3;
+  else
+    value = 0;
+  break;
+
+ case RTX_OBJ:
+  /* Complex expressions should be the first, so decrease priority
+     of objects.  */
+  value = -1;
+  break;
 
-    case RTX_OBJ:
-      /* Complex expressions should be the first, so decrease priority
-         of objects.  Prefer pointer objects over non pointer objects.  */
-      if ((REG_P (op) && REG_POINTER (op))
-  || (MEM_P (op) && MEM_POINTER (op)))
- return -1;
-      return -2;
-
-    case RTX_COMM_ARITH:
-      /* Prefer operands that are themselves commutative to be first.
-         This helps to make things linear.  In particular,
-         (and (and (reg) (reg)) (not (reg))) is canonical.  */
-      return 4;
-
-    case RTX_BIN_ARITH:
-      /* If only one operand is a binary expression, it will be the first
-         operand.  In particular,  (plus (minus (reg) (reg)) (neg (reg)))
-         is canonical, although it will usually be further simplified.  */
-      return 2;
+ case RTX_COMM_ARITH:
+  /* Prefer operands that are themselves commutative to be first.
+     This helps to make things linear.  In particular,
+     (and (and (reg) (reg)) (not (reg))) is canonical.  */
+  value = 4;
+  break;
+
+ case RTX_BIN_ARITH:
+  /* If only one operand is a binary expression, it will be the first
+     operand.  In particular,  (plus (minus (reg) (reg)) (neg (reg)))
+     is canonical, although it will usually be further simplified.  */
+  value = 2;
+  break;
   
-    case RTX_UNARY:
-      /* Then prefer NEG and NOT.  */
-      if (code == NEG || code == NOT)
-        return 1;
+ case RTX_UNARY:
+  /* Then prefer NEG and NOT.  */
+  if (code == NEG || code == NOT)
+    value = 1;
+  else
+    value = 0;
+  break;
 
-    default:
-      return 0;
+ default:
+  value = 0;
+ }
     }
+
+  if (targetm.commutative_operand_precedence)
+    value = targetm.commutative_operand_precedence (op, value);
+
+  return value;
 }
 
 /* Return 1 iff it is necessary to swap operands of commutative operation
Index: config/rs6000/rs6000.c
===================================================================
--- config/rs6000/rs6000.c (revision 148831)
+++ config/rs6000/rs6000.c (working copy)
@@ -912,6 +912,7 @@ static rtx generate_set_vrsave (rtx, rs6
 int easy_vector_constant (rtx, enum machine_mode);
 static rtx rs6000_dwarf_register_span (rtx);
 static void rs6000_init_dwarf_reg_sizes_extra (tree);
+static int rs6000_commutative_operand_precedence (const_rtx, int);
 static rtx rs6000_legitimize_address (rtx, rtx, enum machine_mode);
 static rtx rs6000_legitimize_tls_address (rtx, enum tls_model);
 static void rs6000_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
@@ -1115,6 +1116,10 @@ static const struct attribute_spec rs600
 #undef TARGET_ASM_FUNCTION_EPILOGUE
 #define TARGET_ASM_FUNCTION_EPILOGUE rs6000_output_function_epilogue
 
+#undef TARGET_COMMUTATIVE_OPERAND_PRECEDENCE
+#define TARGET_COMMUTATIVE_OPERAND_PRECEDENCE \
+  rs6000_commutative_operand_precedence
+
 #undef TARGET_LEGITIMIZE_ADDRESS
 #define TARGET_LEGITIMIZE_ADDRESS rs6000_legitimize_address
 
@@ -22240,6 +22245,27 @@ rs6000_memory_move_cost (enum machine_mo
     return 4 + rs6000_register_move_cost (mode, rclass, GENERAL_REGS);
 }
 
+/* Return a value indicating whether OP, an operand of a commutative
+   operation, is preferred as the first or second operand.  The higher
+   the value, the stronger the preference for being the first operand.
+   We use negative values to indicate a preference for the first operand
+   and positive values for the second operand.
+   VALUE is the default precedence for OP; see rtlanal.c:
+   commutative_operand_precendece.  */
+
+static int
+rs6000_commutative_operand_precedence (const_rtx op, int value)
+{
+  /* Prefer pointer objects over non pointer objects.
+     For rationale see PR28690.  */
+  if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ
+      && ((REG_P (op) && REG_POINTER (op))
+  || (MEM_P (op) && MEM_POINTER (op))))
+    --value;
+
+  return value;
+}
+
 /* Returns a code for a target-specific builtin that implements
    reciprocal of the function, or NULL_TREE if not available.  */
 

[Peter Bergner-2]

On Fri, 2009-06-26 at 18:15 +0400, Maxim Kuvyrkov wrote:
> The attached patch fixes PR37053
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37053#c5).
>
> The problem was introduced by patch for PR28690
> (http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00268.html, rtlanal.c
> hunk); this hunk causes ICE on m68k (and, possibly, other) architectures.
[snip]
> My first impulse was to remove the pointer tweak; but, well, if PowerPC
> really benefits from it,


I will say we worked for a long time coming up with the patch we did so
that it didn't affect other arches.  In particular, HJ ran a lot of
performance tests to make sure it didn't degrade performance on x86.
I will also note, that without this patch, SPEC2000 performance degrades
by about 30% across the entire benchmark suite and 500% on galgel on
POWER6, so yeah, we benefit from it. :)


> ..., let's make it a hook.
>
> The attached patch does just that.  Bootstrapped on x86_64-linux-gnu and
> cross-build to powerpc-linux-gnu (with no new warnings in logs); ok to
> apply?

I'll let others comment on whether they want a target hook solution or not.
If they don't, I'm willing to work with you to come up with something that
works for both of us.

In the mean time, I have asked Luis to test your patch on POWER6 to ensure
there are no performance degradations.  It doesn't look like there should
be, but...


Peter

[Maxim Kuvyrkov-2]

Peter Bergner wrote:

> In the mean time, I have asked Luis to test your patch on POWER6 to ensure
> there are no performance degradations.  It doesn't look like there should
> be, but...

The assembler should be exactly the same.  If it isn't, then there's a
bug in the patch.

--
Maxim

[Peter Bergner-2]

On Fri, 2009-06-26 at 20:04 +0400, Maxim Kuvyrkov wrote:
> Peter Bergner wrote:
>
> > In the mean time, I have asked Luis to test your patch on POWER6 to ensure
> > there are no performance degradations.  It doesn't look like there should
> > be, but...
>
> The assembler should be exactly the same.  If it isn't, then there's a
> bug in the patch.

That's exactly what we're testing for.  With your patch, we'll always
decrement the precedence value if we have a pointer.  With the old code,
that occurred only for RTX_OBJ.  Maybe that's the only case were we'll
see pointers, but I personally don't know that for sure.


Peter

[Maxim Kuvyrkov-2]

Peter Bergner wrote:

> On Fri, 2009-06-26 at 20:04 +0400, Maxim Kuvyrkov wrote:
>> Peter Bergner wrote:
>>
>>> In the mean time, I have asked Luis to test your patch on POWER6 to ensure
>>> there are no performance degradations.  It doesn't look like there should
>>> be, but...
>> The assembler should be exactly the same.  If it isn't, then there's a
>> bug in the patch.
>
> That's exactly what we're testing for.  With your patch, we'll always
> decrement the precedence value if we have a pointer.  With the old code,
> that occurred only for RTX_OBJ.  Maybe that's the only case were we'll
> see pointers, but I personally don't know that for sure.

I tried to avoid any differences from the current behavior ...

+static int
+rs6000_commutative_operand_precedence (const_rtx op, int value)
+{
+  /* Prefer pointer objects over non pointer objects.
+     For rationale see PR28690.  */
+  if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ

... and the patch decrements the value only for RTX_OBJs.

+      && ((REG_P (op) && REG_POINTER (op))
+  || (MEM_P (op) && MEM_POINTER (op))))
+    --value;
+
+  return value;
+}

Peter and Luis,

In any case, thanks for testing the patch on PowerPC.

--
Maxim

[Peter Bergner-2]

On Fri, 2009-06-26 at 20:33 +0400, Maxim Kuvyrkov wrote:
> +static int
> +rs6000_commutative_operand_precedence (const_rtx op, int value)
> +{
> +  /* Prefer pointer objects over non pointer objects.
> +     For rationale see PR28690.  */
> +  if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ
>
> ... and the patch decrements the value only for RTX_OBJs.

I'm sorry, I completely missed that hunk.  In that case, it does look
equivalent.  Luis' test runs will verify that.

Peter

[Paolo Bonzini-3]

Peter Bergner wrote:

> On Fri, 2009-06-26 at 18:15 +0400, Maxim Kuvyrkov wrote:
>> The attached patch fixes PR37053
>> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37053#c5).
>>
>> The problem was introduced by patch for PR28690
>> (http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00268.html, rtlanal.c
>> hunk); this hunk causes ICE on m68k (and, possibly, other) architectures.
> [snip]
>> My first impulse was to remove the pointer tweak; but, well, if PowerPC
>> really benefits from it,
>
> I will say we worked for a long time coming up with the patch we did so
> that it didn't affect other arches.  In particular, HJ ran a lot of
> performance tests to make sure it didn't degrade performance on x86.

I cannot say I like Maxim's patch and he knows, but unfortunately, m68k
apparently implements 2-address arithmetic in a different way than x86
(or with other constraint) and we're talking about a lot of ICEs there. :-(

Another possibility, which you would have to try on PowerPC, could be to
limit the change of precedence to before reload.  That would be a
simpler patch and one that does not risk slowing down the compiler that
tiny 0.1% that sums up quickly.  But it would also be pretty magical...

Paolo

[Maxim Kuvyrkov-2]

Paolo Bonzini wrote:

...

> I cannot say I like Maxim's patch and he knows, but unfortunately, m68k
> apparently implements 2-address arithmetic in a different way than x86
> (or with other constraint) and we're talking about a lot of ICEs there. :-(
>
> Another possibility, which you would have to try on PowerPC, could be to
> limit the change of precedence to before reload.

Here is the other patch.  It has a downside that *during* and after
reload the optimal for PowerPC commutativeness is not available, so that
may degrade performance.

Peter, if you find out that this patch doesn't cause any performance
problems, I'm happy to go with it.

Index: gcc-mainline/gcc/rtlanal.c
===================================================================
--- gcc-mainline/gcc/rtlanal.c  (revision 148831)
+++ gcc-mainline/gcc/rtlanal.c  (working copy)
@@ -2936,8 +2936,9 @@ commutative_operand_precedence (rtx op)
      case RTX_OBJ:
        /* Complex expressions should be the first, so decrease priority
           of objects.  Prefer pointer objects over non pointer objects.  */
-      if ((REG_P (op) && REG_POINTER (op))
-         || (MEM_P (op) && MEM_POINTER (op)))
+      if ((!reload_completed && !reload_in_progress)
+         && ((REG_P (op) && REG_POINTER (op))
+             || (MEM_P (op) && MEM_POINTER (op))))
         return -1;
        return -2;

> That would be a
> simpler patch and one that does not risk slowing down the compiler that
> tiny 0.1% that sums up quickly.  But it would also be pretty magical...

That's just a check for a global variable to be NULL on most platforms;
btw, the above patch checks two global variables ;).  On PowerPC the
extra work is justified with performance boost.

--
Maxim

[Richard Sandiford-5]

Maxim Kuvyrkov <maxim@...> writes:
> Paolo Bonzini wrote:
>> I cannot say I like Maxim's patch and he knows, but unfortunately, m68k
>> apparently implements 2-address arithmetic in a different way than x86
>> (or with other constraint) and we're talking about a lot of ICEs there. :-(

FWIW, I don't like the patch either.  Operator precedence seems too
fundemantal to be deferred to a target hook.  But...

>> Another possibility, which you would have to try on PowerPC, could be to
>> limit the change of precedence to before reload.
>
> Here is the other patch.  It has a downside that *during* and after
> reload the optimal for PowerPC commutativeness is not available, so that
> may degrade performance.
>
> Peter, if you find out that this patch doesn't cause any performance
> problems, I'm happy to go with it.
>
> Index: gcc-mainline/gcc/rtlanal.c
> ===================================================================
> --- gcc-mainline/gcc/rtlanal.c  (revision 148831)
> +++ gcc-mainline/gcc/rtlanal.c  (working copy)
> @@ -2936,8 +2936,9 @@ commutative_operand_precedence (rtx op)
>       case RTX_OBJ:
>         /* Complex expressions should be the first, so decrease priority
>            of objects.  Prefer pointer objects over non pointer objects.  */
> -      if ((REG_P (op) && REG_POINTER (op))
> -         || (MEM_P (op) && MEM_POINTER (op)))
> +      if ((!reload_completed && !reload_in_progress)
> +         && ((REG_P (op) && REG_POINTER (op))
> +             || (MEM_P (op) && MEM_POINTER (op))))
>          return -1;
>         return -2;
>
>> That would be a
>> simpler patch and one that does not risk slowing down the compiler that
>> tiny 0.1% that sums up quickly.  But it would also be pretty magical...

...I don't like this any better.  We can process constraints before reload too,
and having different rules for "%" at different stages seems like a bad idea.

I always assumed that one of the main uses of "%" was precisely to allow
regs and mems to be swapped in order to satisfy constraints (which if I've
understood correctly is all that m68k seems to be trying to do).  Declaring
a non-pointer reg + a pointer mem to be noncanonical rtl seems far too strong
at any stage of compilation.  I know 28690 was a serious performance problem,
but I think we need to consider handling this in another way, separating out
"try to achieve this" from "non-canonical".

Which isn't very constructive, sorry.

Richard

[Paolo Bonzini-3]

> I always assumed that one of the main uses of "%" was precisely to allow
> regs and mems to be swapped in order to satisfy constraints (which if I've
> understood correctly is all that m68k seems to be trying to do).  Declaring
> a non-pointer reg + a pointer mem to be noncanonical rtl seems far too strong
> at any stage of compilation.  I know 28690 was a serious performance problem,
> but I think we need to consider handling this in another way, separating out
> "try to achieve this" from "non-canonical".

Also FWIW, this is exactly how I feel.  I don't have big problems with
Maxim's second patch, but only because '%' is basically unused except
for some optimizations within reload.  As Maxim taught me (either I
remembered wrong or I was really convinced this was not the case), % is
not used for matching, so it only really applies whenever two things
have the same precedence in the first place.

A third patch could be to extend the usage of '%' but that's probably
not a good idea either (what happens if the inverted pair does not
satisfy the predicates?) and would require considerably more surgery.

I wonder how much of the performance can be kept on Power6 if you just
attack PR28690 within md_reorg.  The only problem would be if the index
was allocated in r0.

Paolo

[Peter Bergner-2]

On Sat, 2009-06-27 at 13:07 +0200, Paolo Bonzini wrote:
> I wonder how much of the performance can be kept on Power6 if you just
> attack PR28690 within md_reorg.  The only problem would be if the index
> was allocated in r0.

What exactly did you have in mind here?

Peter

[Paolo Bonzini-3]

Peter Bergner wrote:
> On Sat, 2009-06-27 at 13:07 +0200, Paolo Bonzini wrote:
>> I wonder how much of the performance can be kept on Power6 if you just
>> attack PR28690 within md_reorg.  The only problem would be if the index
>> was allocated in r0.
>
> What exactly did you have in mind here?

Look into each MEM looking for PLUSes with the operands in the wrong
order.  Or:

static bool
is_pointer_object (rtx x)
{
   if (REG_P (x))
     return REG_POINTER (x);
   if (MEM_P (x))
     return MEM_POINTER (x);
   return false;
}

/* We are inside a MEM, find PLUSes were the index comes first and
    invert the base and index.  DATA points to the mem within which
    *PX lies, which is used to validate the address.  */

static int
find_plus_and_frob_them (rtx *px, void *data)
{
   if (GET_CODE (x) == PLUS
       && OBJECT_P (XEXP (x, 1))
       && OBJECT_P (XEXP (x, 0))
       && is_pointer_object (XEXP (x, 1))
       && !is_pointer_object (XEXP (x, 0))
     {
       rtx mem = data;
       rtx t = XEXP (x, 0);
       XEXP (x, 0) = XEXP (x, 1);
       XEXP (x, 1) = t;
       gcc_assert (!swap_commutative_operands_p (XEXP (x, 0),
                                                XEXP (x, 1));
       if (!strict_memory_address_p (GET_MODE (mem), XEXP (mem, 0)))
         {
           /* Too bad, probably XEXP (x, 0) is r0 and cannot be made into
              an index.  Undo.  */
           XEXP (x, 1) = XEXP (x, 0);
           XEXP (x, 0) = t;
         }
     }
   return 0;
}

/* Look for memory operands and apply find_plus_and_frob_them to their
    addresses.  */
static int
find_mems_and_frob_them (rtx *px, void *data)
{
   if (GET_CODE (*px) == MEM)
     {
       for_each_rtx (&XEXP (*px, 0), find_plus_and_frob_them, *px);
       return -1;
     }
   return 0;
}

and then in a machine-dependent reorg pass (which rs6000 currently does
not have):

FOR_EACH_BB (bb)
   FOR_BB_INSNS (bb, insn)
     if (INSN_P (insn))
       for_each_rtx (&PATTERN (insn), find_mems_and_frob_them, NULL);

[Peter Bergner-2]

On Sat, 2009-06-27 at 16:33 +0200, Paolo Bonzini wrote:
> and then in a machine-dependent reorg pass (which rs6000 currently does
> not have):
>
> FOR_EACH_BB (bb)
>    FOR_BB_INSNS (bb, insn)
>      if (INSN_P (insn))
>        for_each_rtx (&PATTERN (insn), find_mems_and_frob_them, NULL);

Originally, we tried just checking the REG_POINTER attribute when we
generated the asm for loads and stores, but there were so many passes
that didn't preserve the REG_POINTER/MEM_POINTER attribute, it just
didn't work.  We have since fixed a few of them, so hopefully at reorg
time, they're still there.  I'll have a go with your suggestion and get
back to you.  Thanks.


Peter

[Luis Machado-5]

Hi folks,

I gave it a try with Maxim's patch from ticket #37053 and it degraded
performance pretty badly (> 30%), and galgel degraded more than 75%.
Both 32-bit and 64-bit saw degradations.

Worth noticing that the patch posted here didn't degrade performance (i
heard that's Paolo's idea):

http://gcc.gnu.org/ml/gcc-patches/2009-06/msg02038.html

Regards,
Luis

On Fri, 2009-06-26 at 11:40 -0500, Peter Bergner wrote:

> On Fri, 2009-06-26 at 20:33 +0400, Maxim Kuvyrkov wrote:
> > +static int
> > +rs6000_commutative_operand_precedence (const_rtx op, int value)
> > +{
> > +  /* Prefer pointer objects over non pointer objects.
> > +     For rationale see PR28690.  */
> > +  if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ
> >
> > ... and the patch decrements the value only for RTX_OBJs.
>
> I'm sorry, I completely missed that hunk.  In that case, it does look
> equivalent.  Luis' test runs will verify that.
>
> Peter

[Maxim Kuvyrkov-2]

Luis Machado wrote:
> Hi folks,

Hi Luis,

There are two patches at the moment: the long one (adding target hook)
and the short one (using reload_in_progress and reload_completed).

>
> I gave it a try with Maxim's patch from ticket #37053 and it degraded
> performance pretty badly (> 30%), and galgel degraded more than 75%.
> Both 32-bit and 64-bit saw degradations.

The patch in PR37053 is the one adding target hook.  This patch should
not affect the generated code at all.  If it does, can you please send
the the difference in dumps?  I understand that this patch does not seem
to be a favorite here, but I'm curious where I've made a mistake.

>
> Worth noticing that the patch posted here didn't degrade performance (i
> heard that's Paolo's idea):
>
> http://gcc.gnu.org/ml/gcc-patches/2009-06/msg02038.html

This is the short patch and it's good news it didn't degrade.

--
Maxim

[Maxim Kuvyrkov-2]

Maxim Kuvyrkov wrote:
> Luis Machado wrote:

...

>> I gave it a try with Maxim's patch from ticket #37053 and it degraded
>> performance pretty badly (> 30%), and galgel degraded more than 75%.
>> Both 32-bit and 64-bit saw degradations.
>
> The patch in PR37053 is the one adding target hook.  This patch should
> not affect the generated code at all.  If it does, can you please send
> the the difference in dumps?  I understand that this patch does not seem
> to be a favorite here, but I'm curious where I've made a mistake.

The mistake is in rs6000_commutative_operand_precedence(); the
precedence value should be decrement for everything but the pointers,
current code does exactly the opposite.

The hook implementation should be:

static int
rs6000_commutative_operand_precedence (const_rtx op, int value)
{
   /* Prefer pointer objects over non pointer objects.
      For rationale see PR28690.  */
   if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ
       && ((REG_P (op) && REG_POINTER (op))
          || (MEM_P (op) && MEM_POINTER (op))))
     /* value = -1 */;
   else
     /* value = -2 */
     --value;

   return value;
}

--
Maxim

[Luis Machado-5]

Hi,

On Mon, 2009-06-29 at 16:41 +0400, Maxim Kuvyrkov wrote:

> Maxim Kuvyrkov wrote:
> > Luis Machado wrote:
>
> ...
>
> >> I gave it a try with Maxim's patch from ticket #37053 and it degraded
> >> performance pretty badly (> 30%), and galgel degraded more than 75%.
> >> Both 32-bit and 64-bit saw degradations.
> >
> > The patch in PR37053 is the one adding target hook.  This patch should
> > not affect the generated code at all.  If it does, can you please send
> > the the difference in dumps?  I understand that this patch does not seem
> > to be a favorite here, but I'm curious where I've made a mistake.
>
> The mistake is in rs6000_commutative_operand_precedence(); the
> precedence value should be decrement for everything but the pointers,
> current code does exactly the opposite.
>
> The hook implementation should be:
>
> static int
> rs6000_commutative_operand_precedence (const_rtx op, int value)
> {
>    /* Prefer pointer objects over non pointer objects.
>       For rationale see PR28690.  */
>    if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ
>        && ((REG_P (op) && REG_POINTER (op))
>  || (MEM_P (op) && MEM_POINTER (op))))
>      /* value = -1 */;
>    else
>      /* value = -2 */
>      --value;
>
>    return value;
> }
>
> --
> Maxim

I'll tweak that bit and will try again. I already see good numbers for
galgel.

Luis

[Luis Machado-5]

Hi,

On Mon, 2009-06-29 at 10:03 -0300, Luis Machado wrote:

> Hi,
>
> On Mon, 2009-06-29 at 16:41 +0400, Maxim Kuvyrkov wrote:
> > Maxim Kuvyrkov wrote:
> > > Luis Machado wrote:
> >
> > ...
> >
> > >> I gave it a try with Maxim's patch from ticket #37053 and it degraded
> > >> performance pretty badly (> 30%), and galgel degraded more than 75%.
> > >> Both 32-bit and 64-bit saw degradations.
> > >
> > > The patch in PR37053 is the one adding target hook.  This patch should
> > > not affect the generated code at all.  If it does, can you please send
> > > the the difference in dumps?  I understand that this patch does not seem
> > > to be a favorite here, but I'm curious where I've made a mistake.
> >
> > The mistake is in rs6000_commutative_operand_precedence(); the
> > precedence value should be decrement for everything but the pointers,
> > current code does exactly the opposite.
> >
> > The hook implementation should be:
> >
> > static int
> > rs6000_commutative_operand_precedence (const_rtx op, int value)
> > {
> >    /* Prefer pointer objects over non pointer objects.
> >       For rationale see PR28690.  */
> >    if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ
> >        && ((REG_P (op) && REG_POINTER (op))
> >  || (MEM_P (op) && MEM_POINTER (op))))
> >      /* value = -1 */;
> >    else
> >      /* value = -2 */
> >      --value;
> >
> >    return value;
> > }
> >
> > --
> > Maxim
>
> I'll tweak that bit and will try again. I already see good numbers for
> galgel.
>
> Luis

With the fix, most of the benchmarks seem to be back to normal, but swim
(both 32/64-bit) and parser (32-bit) went down a bit, about 5%.

Luis

[Paolo Bonzini-2]

> With the fix, most of the benchmarks seem to be back to normal, but swim
> (both 32/64-bit) and parser (32-bit) went down a bit, about 5%.

Could it be some cache fluctuation?  The patch is either all or nothing,
if it works properly it should give the exact same executable.

Paolo

[Luis Machado-5]

On Mon, 2009-06-29 at 19:18 +0200, Paolo Bonzini wrote:
> > With the fix, most of the benchmarks seem to be back to normal, but swim
> > (both 32/64-bit) and parser (32-bit) went down a bit, about 5%.
>
> Could it be some cache fluctuation?  The patch is either all or nothing,
> if it works properly it should give the exact same executable.

> Paolo

True. Additional runs showed good results for those benchmarks. So it
looks like there is no degradation associated with the fixed version of
Maxim's patch.

Luis