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

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Jul 06, 2007 at 10:07:47AM -0500, Peter Bergner wrote:
> On Thu, 2007-07-05 at 23:30 -0700, Ian Lance Taylor wrote:
> > Peter Bergner <bergner@vnet.ibm.com> writes:
> > > Ian, on IRC you mentioned you thought the following patch was probably
> > > ok to commit.
> > > 
> > >     http://gcc.gnu.org/ml/gcc-patches/2007-06/msg01493.html
> > > 
> > > Was that a formal approval?
> > 
> > Yes, this is OK.
> 
> Great, thanks!  Given it's the weekend and I won't be around much, I'm
> going to wait until Monday before committing this just in case there's
> any fallout on ports other than the tested {power6464,x86_64}-linux.

A modified version of the above patch (conditionalized using a TARGET
macro, so that it doesn't affect non-ppc or non-power6 tuning on ppc)
on redhat/gcc-4_1-branch caused a regression, see
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=247256
for details.

While I haven't tried to reproduce this with your patch on the trunk,
I think it is fairly likely it will hit the same problem, either on
the same testcase or on some other one.
When swap_commutative_operand_p (which was always false before when
both arguments were REGs), but now prefers lower REGNOs first
fights against simplify_plus_minus which on the other side prefers
whatever was originally first in such cases, we can end up with
endless recursion.

The following (so far only on the testcase tested) patch ought to make
the two match:

2007-07-09  Jakub Jelinek  <jakub@redhat.com>

	* simplify-rtx.c (simplify_plus_minus_op_data_cmp): If both operands
	are REGs, sort lower REGNOs first.

--- gcc/simplify-rtx.c.jj	2006-08-11 17:32:05.000000000 +0200
+++ gcc/simplify-rtx.c	2007-07-09 22:53:26.000000000 +0200
@@ -2608,6 +2608,12 @@ simplify_plus_minus_op_data_cmp (const v
 	    - commutative_operand_precedence (d1->op));
   if (result)
     return result;
+
+  /* Group together equal REGs to do more simplification.  */
+  if (TARGET_INDEX_OPERAND_FIRST && REG_P (d1->op) && REG_P (d2->op)
+      && REGNO (d1->op) != REGNO (d2->op))
+    return REGNO (d1->op) - REGNO (d2->op);
+
   return d1->ix - d2->ix;
 }
 

Additionally, I think simplify_associative_operation can use some cleanup,
particularly simplify_binary_operation calls itself
swap_commutative_operands_p on the two operands passed to it and swaps the
arguments, so calling it in simplify_associative_operation again
is just a waste of CPU cycles.  Furthermore, even if that wasn't true,
swap_commutative_operands_p (XEXP (op0, 1), op1) is known to be
false in the first case (if it is true we do something different and
unconditionally return earlier).

2007-07-09  Jakub Jelinek  <jakub@redhat.com>

	* simplify-rtx.c (simplify_associative_operation): Don't
	reorder simplify_binary_operation arguments.

--- gcc/simplify-rtx.c	2007-06-25 22:22:37.000000000 +0200
+++ gcc/simplify-rtx.c	2007-07-09 22:15:30.000000000 +0200
@@ -1499,16 +1499,12 @@ simplify_associative_operation (enum rtx
 	}
 
       /* Attempt to simplify "(a op b) op c" as "a op (b op c)".  */
-      tem = swap_commutative_operands_p (XEXP (op0, 1), op1)
-	    ? simplify_binary_operation (code, mode, op1, XEXP (op0, 1))
-	    : simplify_binary_operation (code, mode, XEXP (op0, 1), op1);
+      tem = simplify_binary_operation (code, mode, XEXP (op0, 1), op1);
       if (tem != 0)
         return simplify_gen_binary (code, mode, XEXP (op0, 0), tem);
 
       /* Attempt to simplify "(a op b) op c" as "(a op c) op b".  */
-      tem = swap_commutative_operands_p (XEXP (op0, 0), op1)
-	    ? simplify_binary_operation (code, mode, op1, XEXP (op0, 0))
-	    : simplify_binary_operation (code, mode, XEXP (op0, 0), op1);
+      tem = simplify_binary_operation (code, mode, XEXP (op0, 0), op1);
       if (tem != 0)
         return simplify_gen_binary (code, mode, tem, XEXP (op0, 1));
     }


	Jakub