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

[Peter Bergner <bergner at vnet dot ibm dot com>]

On Sun, 2007-07-15 at 06:35 -0700, H.J. Lu wrote:
> 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

Jakub already posted a fix for that GCC 4.1 bug.  I mentioned it in
another note, but I'll repeat is again, mainline doesn't suffer from
that bug.



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

I'm a little confused here.  What source code base did you apply the
patch you posted to?  Plain mainline?  Mainline with my submitted (and
approved but not yet committed) patch?  Mainline plus some other
modified patch you came up with?  It's hard to evaluate what could
be going wrong without knowing exactly what you're testing.

You never mentioned any testsuite failures before, so I assume that
testcase passes using mainline + my patch?  I didn't hit any testsuite
failures while testing on x86_64.



> 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
[snip]
> But Peter, you have to tell me which one works for Power 6.

See my previous email.




> 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.

A simpler change (assumes my REGNO sorting swap_commutative_operands_p
change has been applied) would be to simply replace all calls to
simplify_plus_minus_op_data_cmp with swap_commutative_operands_p
like the code below.

I'll note that the behavior of the code is changed slightly, but I think in a
good way.  With the original code, if the operands are already in canonical
form and two or more of the operands have the same precedence, then we won't
actually end up swapping anything, but given that:

	if (simplify_plus_minus_op_data_cmp (&ops[j], &ops[i]) < 0)
 	  continue;

doesn't "continue" for operands with the same precedence (ie, return val is 0),
then we'll end up falling through, setting canonicalized=1 and then not swap
anything because the while loop exits because it's testing for > 0. In this
particular case, we'll fail to use the shortcut located after the loop:

      /* This is only useful the first time through.  */
      if (!canonicalized)
	return NULL_RTX;


Peter


2007-07-15  Peter Bergner <bergner@us.ibm.com>

	* simplify-rtx.c (simplify_plus_minus_op_data_cmp): Delete.
	(simplify_plus_minus): Replace calls to simplify_plus_minus_op_data_cmp
	with swap_commutative_operands_p.

Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 126496)
+++ simplify-rtx.c	(working copy)
@@ -52,7 +52,6 @@ Software Foundation, 51 Franklin Street,
 
 static rtx neg_const_int (enum machine_mode, rtx);
 static bool plus_minus_operand_p (rtx);
-static int simplify_plus_minus_op_data_cmp (const void *, const void *);
 static rtx simplify_plus_minus (enum rtx_code, enum machine_mode, rtx, rtx);
 static rtx simplify_immed_subreg (enum machine_mode, rtx, enum machine_mode,
 				  unsigned int);
@@ -3313,25 +3308,6 @@ struct simplify_plus_minus_op_data
   short neg;
 };
 
-static int
-simplify_plus_minus_op_data_cmp (const void *p1, const void *p2)
-{
-  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;
-}
-
 static rtx
 simplify_plus_minus (enum rtx_code code, enum machine_mode mode, rtx op0,
 		     rtx op1)
@@ -3473,14 +3449,14 @@ simplify_plus_minus (enum rtx_code code,
         {
           struct simplify_plus_minus_op_data save;
           j = i - 1;
-          if (simplify_plus_minus_op_data_cmp (&ops[j], &ops[i]) < 0)
+	  if (!swap_commutative_operands_p (opj[j], ops[i]))
 	    continue;
 
           canonicalized = 1;
           save = ops[i];
           do
 	    ops[j + 1] = ops[j];
-          while (j-- && simplify_plus_minus_op_data_cmp (&ops[j], &save) > 0);
+          while (j-- && swap_commutative_operands_p (ops[j], save));
           ops[j + 1] = save;
         }