Re: [PATCH] Fix PR middle-end/28690, indexed load/store performance + reload bug

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

On Thu, Jan 11, 2007 at 08:40:43AM +0100, Paolo Bonzini wrote:
>> Sorry for asking again, but "pointless" in that the operands should
>> already be in the order we want so we can remove completely the call
>> to swap_commutative_operands_with_target, or "pointless" as in the
>> extra heuristic within swap_commutative_operands_with_target is no
>> longer needed and we can simply call swap_commutative_operands_p
>> instead?  I'm assuming you mean the former rather than the later.
> 
> No, I mean the latter.  Sorry for not being clear.

Ok, using the following patch on your testcase, I don't see any code
differences between using mainline versus mainline plus this patch
for both -O1 and -O2.  Can you give this patch a try to verify it
doesn't affect x86 SPEC scores in a negative way?

Peter


2007-01-11  Peter Bergner  <bergner@vnet.ibm.com>

	* optabs.c (expand_binop): Use swap_commutative_operands_p
	to order operands.
	(swap_commutative_operands_with_target): Delete.
	* rtlanal.c (commutative_operand_precedence): Prefer both REG_POINTER
	and MEM_POINTER operands over REG and MEM operands.
	(swap_commutative_operands_p): Change return value to bool.
	* rtl.h: Update the corresponding prototype.
	* tree-ssa-address.c (gen_addr_rtx): Use simplify_gen_binary
	instead of gen_rtx_PLUS.


Index: optabs.c
===================================================================
--- optabs.c	(revision 120647)
+++ optabs.c	(working copy)
@@ -1193,29 +1193,6 @@ expand_simple_binop (enum machine_mode m
   return expand_binop (mode, binop, op0, op1, target, unsignedp, methods);
 }
 
-/* Return whether OP0 and OP1 should be swapped when expanding a commutative
-   binop.  Order them according to commutative_operand_precedence and, if
-   possible, try to put TARGET or a pseudo first.  */
-static bool
-swap_commutative_operands_with_target (rtx target, rtx op0, rtx op1)
-{
-  int op0_prec = commutative_operand_precedence (op0);
-  int op1_prec = commutative_operand_precedence (op1);
-
-  if (op0_prec < op1_prec)
-    return true;
-
-  if (op0_prec > op1_prec)
-    return false;
-
-  /* With equal precedence, both orders are ok, but it is better if the
-     first operand is TARGET, or if both TARGET and OP0 are pseudos.  */
-  if (target == 0 || REG_P (target))
-    return (REG_P (op1) && !REG_P (op0)) || target == op1;
-  else
-    return rtx_equal_p (op1, target);
-}
-
 
 /* Generate code to perform an operation specified by BINOPTAB
    on operands OP0 and OP1, with result having machine-mode MODE.
@@ -1292,7 +1269,7 @@ expand_binop (enum machine_mode mode, op
     {
       commutative_op = 1;
 
-      if (swap_commutative_operands_with_target (target, op0, op1))
+      if (swap_commutative_operands_p (op0, op1))
 	{
 	  temp = op1;
 	  op1 = op0;
Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 120647)
+++ rtlanal.c	(working copy)
@@ -2776,9 +2776,9 @@ commutative_operand_precedence (rtx op)
   
   /* 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);
 
@@ -2786,26 +2786,29 @@ commutative_operand_precedence (rtx op)
     {
     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.
@@ -2832,7 +2835,7 @@ commutative_operand_precedence (rtx op)
 /* 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)
Index: tree-ssa-address.c
===================================================================
--- tree-ssa-address.c	(revision 120647)
+++ tree-ssa-address.c	(working copy)
@@ -125,7 +125,7 @@ gen_addr_rtx (rtx symbol, rtx base, rtx 
   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 120647)
+++ rtl.h	(working copy)
@@ -1680,7 +1680,7 @@ 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 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);