This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PING][PATCH][REVISED] Fix PR middle-end/PR28690, modify swap_commutative_operands_p


On Sat, Jul 14, 2007 at 09:56:18AM -0700, H.J. Lu wrote:
> On Sat, Jul 14, 2007 at 09:48:44AM -0700, H.J. Lu wrote:
> > On Sat, Jul 14, 2007 at 10:38:34AM -0500, Peter Bergner wrote:
> > > On Sat, 2007-07-14 at 07:25 -0700, H.J. Lu wrote:
> > > > Peter, will gcc-PR28690-5.patch solve your problem?
> > > 
> > > I'm not sure you actually looked at my previous post or the patch.
> > > By leaving out the changes that adjust all of the precedence values
> > > by -3 but leaving in this part:
> > > 
> > > -      return -1;
> > > +      if (REG_P (op))
> > > +       return (REG_POINTER (op)) ? -1 : -3;
> > > +      else
> > > +       return (MEM_P (op) && MEM_POINTER (op)) ? -2 : -4;
> > > 
> > > You have now just changed the precedence sorting in an unwanted way.
> > > For example, RTX_EXTRA which has a precedence of -2 (-5 with my patch)
> > > and previously always had a lower precedence than all RTX_OBJs, now has
> > > the same precedence or higher precedence than REG's and MEM's.
> > > We don't want that!
> > > 
> > > As I mentioned in the previous post, the adjustment of the precedence
> > > values by -3 is totally cosmetic and will not change the behavior of
> > > the compiler in any way, so I'm curious why you even attempted to leave
> > > that part out.
> > > 
> > > Peter
> > > 
> > 
> > Something like this? I have a question on this part
> > 
> > 
> > +      if (REG_P (op))
> > +	return (REG_POINTER (op)) ? -1 : -3;
> > +      else
> > +	return (MEM_P (op) && MEM_POINTER (op)) ? -2 : -4;
> > 
> > 
> > So the order Power 6 prefers is
> > 
> > 1. REG_POINTER
> > 2. MEM_POINTER
> > 3. REG_P
> > 4. MEM_P
> > 
> > Is that correct? Will
> > 
> > 1. REG_POINTER
> > 2. MEM_POINTER
> > 3. REG_P/MEM_P
> > 
> > work for you.
> > 
> 
> Also will
> 
> 1. REG_POINTER/MEM_POINTER
> 2. REG_P/MEM_P
> 
> work for you.
> 

This is the second part of the original patch. I merged it with
simplify_plus_minus_op_data_cmp. I hope I didn't make any mistakes.
This may be a real bug fix for

http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=247256

However, this patch causes a performance regresion on x86. It fails
gcc.target/i386/387-11.c. Before the patch:

foo:
        movsd   %xmm0, -8(%rsp)
        fldl    -8(%rsp)
        movsd   %xmm1, -8(%rsp)
        fldl    -8(%rsp)
        fmulp   %st, %st(1)
        fstpl   -8(%rsp)
        movsd   -8(%rsp), %xmm0
        ret

After the patch,

foo:
        movsd   %xmm0, -8(%rsp)
        fldl    -8(%rsp)
        movsd   %xmm1, -8(%rsp)
        fchs
        fldl    -8(%rsp)
        fmulp   %st, %st(1)
        fchs
        fstpl   -8(%rsp)
        movsd   -8(%rsp), %xmm0
        ret

I believe we should evaluate 2 parts of the original patch
independently and keep the commutative_operand_precedence change
to minimum. That means we should first try

1. REG_POINTER/MEM_POINTER
2. REG_P/MEM_P

then

1. REG_POINTER
2. MEM_POINTER
3. REG_P/MEM_P

and then

1. REG_POINTER
2. MEM_POINTER
3. REG_P
4. MEM_P

But Peter, you have to tell me which one works for Power 6.

Thanks.


H.J.
----
2007-07-14  Peter Bergner  <bergner@vnet.ibm.com>
	    H.J. Lu  <hongjiu.lu@intel.com>

	* rtl.h (swap_commutative_operands_p): Change return value
	to bool.
	(commutative_operand_precedence_cmp): New.

	* rtlanal.c (commutative_operand_precedence_cmp): New.
	(swap_commutative_operands_p): Change return value to bool.
	Call commutative_operand_precedence_cmp. 

	* simplify-rtx.c (simplify_plus_minus_op_data_cmp): Call
	commutative_operand_precedence_cmp.

--- gcc/rtl.h.reg-sort	2007-07-14 06:49:23.000000000 -0700
+++ gcc/rtl.h	2007-07-14 21:43:43.000000000 -0700
@@ -1690,7 +1690,8 @@ extern int reg_referenced_p (rtx, rtx);
 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 int commutative_operand_precedence_cmp (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);
--- gcc/rtlanal.c.reg-sort	2007-07-14 06:57:08.000000000 -0700
+++ gcc/rtlanal.c	2007-07-14 22:05:31.000000000 -0700
@@ -2929,14 +2929,30 @@ commutative_operand_precedence (rtx op)
     }
 }
 
-/* Return 1 iff it is necessary to swap operands of commutative operation
-   in order to canonicalize expression.  */
+/* Return Y precedence - X precedence.  */
 
 int
+commutative_operand_precedence_cmp (rtx x, rtx y)
+{
+  int result = (commutative_operand_precedence (y)
+		- commutative_operand_precedence (x));
+  if (result)
+    return result;
+
+  /* Group together equal REGs to do more simplification.  */
+  if (REG_P (x) && REG_P (y))
+    return REGNO (x) - REGNO (y);
+  else
+    return 0;
+}
+
+/* Return true iff it is necessary to swap operands of commutative
+   operation in order to canonicalize expression.  */
+
+bool
 swap_commutative_operands_p (rtx x, rtx y)
 {
-  return (commutative_operand_precedence (x)
-	  < commutative_operand_precedence (y));
+  return commutative_operand_precedence_cmp (x, y) > 0;
 }
 
 /* Return 1 if X is an autoincrement side effect and the register is
--- gcc/simplify-rtx.c.reg-sort	2007-07-12 15:08:48.000000000 -0700
+++ gcc/simplify-rtx.c	2007-07-14 22:05:28.000000000 -0700
@@ -3318,18 +3318,7 @@ simplify_plus_minus_op_data_cmp (const v
 {
   const struct simplify_plus_minus_op_data *d1 = p1;
   const struct simplify_plus_minus_op_data *d2 = p2;
-  int result;
-
-  result = (commutative_operand_precedence (d2->op)
-	    - commutative_operand_precedence (d1->op));
-  if (result)
-    return result;
-
-  /* Group together equal REGs to do more simplification.  */
-  if (REG_P (d1->op) && REG_P (d2->op))
-    return REGNO (d1->op) - REGNO (d2->op);
-  else
-    return 0;
+  return commutative_operand_precedence_cmp (d1->op, d2->op);
 }
 
 static rtx


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]