[REVISED][PATCH/RFT] Fix PR middle-end/PR28690, modify swap_commutative_operands_p

[Peter Bergner]

On Mon, Jun 04, 2007 at 02:40:05PM -0500, Peter Bergner wrote:
> On Sun, May 06, 2007 at 01:21:16PM -0700, H. J. Lu wrote:
> > Here are what I got on Core 2 Duo with -O2 -ffast-math:
> > 
> > 2. SPEC CPU 2000 cores
> [snip]
> > 168.wupwise                      -6.56276%
> 
> Well, we found a Core 2 Duo box here and we're not seeing as big
> a degradation as you are (-3.9%), but we seem to have narrowed
> down one slowdown and created a C testcase that seems to mimic the
> slowdown.  Can you confirm that the testcase below runs slower
> for you too.  Also, we don't know the x86/x86_64 arch that well,
> so can you tell us why the code is running slower so I can attempt
> to modify the patch so it doesn't?  Thanks.


On Fri, Jun 08, 2007 at 03:26:17PM -0700, H. J. Lu wrote:
> With revision 125027, I got the following on Core 2 Duo 64bit:
> 
> 			(r125027 + patch - r125027)/125027

Strange how the numbers are jumping around from your previous
numbers given the newer mainline revision.  Anyway, having
looked at the wupwise slowdown from before, it seems the
(set X (X Y)) swapping in swap_commutative_operands_with_target
is still important, so I reverted that part of the patch and
early indications are that it doesn't seem to affect our POWER6
numbers and it seems to fix the wupwise slowdown you were seeing.
So can I ask you to try the updated patch below and see how that
compares with the previous one?  Thanks.

Peter



Index: optabs.c
===================================================================
--- optabs.c	(revision 125171)
+++ optabs.c	(working copy)
@@ -4012,9 +4012,11 @@
   /* Swap operands and condition to ensure canonical RTL.  */
   if (swap_commutative_operands_p (x, y))
     {
-      /* If we're not emitting a branch, this means some caller
-         is out of sync.  */
-      gcc_assert (label);
+      /* If we're not emitting a branch, callers are required to pass
+	 operands in an order conforming to canonical RTL.  We relax this
+	 for commutative comparsions so callers using EQ don't need to do
+	 swapping by hand.  */
+      gcc_assert (label || (comparison == swap_condition (comparison)));
 
       op0 = y, op1 = x;
       comparison = swap_condition (comparison);
Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 125171)
+++ rtlanal.c	(working copy)
@@ -2868,9 +2868,9 @@
   
   /* Constants always come the second operand.  Prefer "nice" constants.  */
   if (code == CONST_INT)
-    return -7;
+    return -10;
   if (code == CONST_DOUBLE)
-    return -6;
+    return -9;
   op = avoid_constant_pool_reference (op);
   code = GET_CODE (op);
 
@@ -2878,26 +2878,29 @@
     {
     case RTX_CONST_OBJ:
       if (code == CONST_INT)
-        return -5;
+        return -8;
       if (code == CONST_DOUBLE)
-        return -4;
-      return -3;
+        return -7;
+      return -6;
 
     case RTX_EXTRA:
       /* SUBREGs of objects should come second.  */
       if (code == SUBREG && OBJECT_P (SUBREG_REG (op)))
-        return -2;
+        return -5;
 
       if (!CONSTANT_P (op))
         return 0;
       else
 	/* As for RTX_CONST_OBJ.  */
-	return -3;
+	return -6;
 
     case RTX_OBJ:
       /* Complex expressions should be the first, so decrease priority
          of objects.  */
-      return -1;
+      if (REG_P (op))
+	return (REG_POINTER (op)) ? -1 : -3;
+      else
+	return (MEM_P (op) && MEM_POINTER (op)) ? -2 : -4;
 
     case RTX_COMM_ARITH:
       /* Prefer operands that are themselves commutative to be first.
@@ -2924,11 +2927,19 @@
 /* Return 1 iff it is necessary to swap operands of commutative operation
    in order to canonicalize expression.  */
 
-int
+bool
 swap_commutative_operands_p (rtx x, rtx y)
 {
-  return (commutative_operand_precedence (x)
-	  < commutative_operand_precedence (y));
+  int result = (commutative_operand_precedence (x)
+		- commutative_operand_precedence (y));
+  if (result)
+    return result < 0;
+
+  /* Group together equal REGs to do more simplification.  */
+  if (REG_P (x) && REG_P (y))
+    return REGNO (x) > REGNO (y);
+
+  return false;
 }
 
 /* Return 1 if X is an autoincrement side effect and the register is
Index: tree-ssa-address.c
===================================================================
--- tree-ssa-address.c	(revision 125171)
+++ tree-ssa-address.c	(working copy)
@@ -125,7 +125,7 @@
   if (base)
     {
       if (*addr)
-	*addr = gen_rtx_PLUS (Pmode, *addr, base);
+	*addr = simplify_gen_binary (PLUS, Pmode, base, *addr);
       else
 	*addr = base;
     }
Index: rtl.h
===================================================================
--- rtl.h	(revision 125171)
+++ rtl.h	(working copy)
@@ -1670,7 +1670,7 @@
 extern int reg_used_between_p (rtx, rtx, rtx);
 extern int reg_set_between_p (rtx, rtx, rtx);
 extern int commutative_operand_precedence (rtx);
-extern int swap_commutative_operands_p (rtx, rtx);
+extern bool swap_commutative_operands_p (rtx, rtx);
 extern int modified_between_p (rtx, rtx, rtx);
 extern int no_labels_between_p (rtx, rtx);
 extern int modified_in_p (rtx, rtx);