[PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

Hello,

The attached patch fixes PR37053 
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37053#c5).

The problem was introduced by patch for PR28690 
(http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00268.html, rtlanal.c 
hunk); this hunk causes ICE on m68k (and, possibly, other) architectures.

Analysis:

Let's take instruction

(insn 309 2675 2677 36 postreload.c:446 (set (reg:SI 0 %d0)
        (plus:SI (mem/f:SI (reg:SI 8 %a0) [0 S4 A16])
            (reg:SI 0 %d0))) 132 {*addsi3_internal} (nil))

Before the patch reload was free to swap second (mem) and third (reg) 
operand and match the pattern to the 4th alternative of

(define_insn "*addsi3_internal"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,d,a")
        (plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0,0")
                 (match_operand:SI 2 "general_src_operand" 
"dIKLT,rJK,a,mSrIKLT,mSrIKLs")))]

After the patch, due to (mem) being a pointer, reload can no longer swap 
the two operands and match the insn.

My first impulse was to remove the pointer tweak; but, well, if PowerPC 
really benefits from it, let's make it a hook.

The attached patch does just that.  Bootstrapped on x86_64-linux-gnu and 
cross-build to powerpc-linux-gnu (with no new warnings in logs); ok to 
apply?


Thanks,

--
Maxim

2009-06-24  Maxim Kuvyrkov  <maxim@codesourcery.com>

	* doc/tm.texi (TARGET_COMMUTATIVE_OPERAND_PRECEDENCE): Document.
	* target.h (commutative_operand_precedence): New hook.
	* target-def.h (TARGET_COMMUTATIVE_OPERAND_PRECEDENCE): Define
	the default.
	* rtlanal.c (commutative_operand_precedence): Use the new hook.
	Move favoring of pointer objects to ...
	* config/rs6000/rs6000.c (rs6000_commutative_operand_precedence):
	... here.  Implement hook.

Index: doc/tm.texi
===================================================================
--- doc/tm.texi	(revision 148831)
+++ doc/tm.texi	(working copy)
@@ -10592,6 +10592,18 @@ to analyze inner elements of @var{x} in 
 passed along.
 @end deftypefn
 
+@deftypefn {Target Hook} int TARGET_COMMUTATIVE_OPERAND_PRECEDENCE (const_rtx @var{op}, int @var{value})
+This target hook returns a value indicating whether @var{OP}, an operand of
+a commutative operation, is preferred as the first or second operand.
+The higher the value, the stronger the preference for being the first operand.
+Negative values are used to indicate a preference for the first operand
+and positive values for the second operand.  Usually the hook will return
+@var{VALUE}, which is the default precedence for @var{OP}
+(see @file{rtlanal.c}:@code{commutative_operand_precedence()}), but sometimes
+backends may wish certain operands to appear at the right places within
+instructions.
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_SET_CURRENT_FUNCTION (tree @var{decl})
 The compiler invokes this hook whenever it changes its current function 
 context (@code{cfun}).  You can define this function if
Index: target.h
===================================================================
--- target.h	(revision 148831)
+++ target.h	(working copy)
@@ -709,6 +709,10 @@ struct gcc_target
      FLAGS has the same meaning as in rtlanal.c: may_trap_p_1.  */
   int (* unspec_may_trap_p) (const_rtx x, unsigned flags);
 
+  /* Return a value indicating whether an operand of a commutative
+     operation is preferred as the first or second operand.  */
+  int (* commutative_operand_precedence) (const_rtx, int);
+
   /* Given a register, this hook should return a parallel of registers
      to represent where to find the register pieces.  Define this hook
      if the register and its mode are represented in Dwarf in
Index: target-def.h
===================================================================
--- target-def.h	(revision 148831)
+++ target-def.h	(working copy)
@@ -509,6 +509,7 @@
 #define TARGET_ALLOCATE_INITIAL_VALUE NULL
 
 #define TARGET_UNSPEC_MAY_TRAP_P default_unspec_may_trap_p
+#define TARGET_COMMUTATIVE_OPERAND_PRECEDENCE NULL
 
 #ifndef TARGET_SET_CURRENT_FUNCTION
 #define TARGET_SET_CURRENT_FUNCTION hook_void_tree
@@ -900,6 +901,7 @@
   TARGET_ADDRESS_COST,				\
   TARGET_ALLOCATE_INITIAL_VALUE,		\
   TARGET_UNSPEC_MAY_TRAP_P,                     \
+  TARGET_COMMUTATIVE_OPERAND_PRECEDENCE,        \
   TARGET_DWARF_REGISTER_SPAN,                   \
   TARGET_INIT_DWARF_REG_SIZES_EXTRA,		\
   TARGET_FIXED_CONDITION_CODE_REGS,		\
Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 148831)
+++ rtlanal.c	(working copy)
@@ -2905,62 +2905,78 @@ int
 commutative_operand_precedence (rtx op)
 {
   enum rtx_code code = GET_CODE (op);
+  int value;
   
   /* Constants always come the second operand.  Prefer "nice" constants.  */
   if (code == CONST_INT)
-    return -8;
-  if (code == CONST_DOUBLE)
-    return -7;
-  if (code == CONST_FIXED)
-    return -7;
-  op = avoid_constant_pool_reference (op);
-  code = GET_CODE (op);
-
-  switch (GET_RTX_CLASS (code))
-    {
-    case RTX_CONST_OBJ:
-      if (code == CONST_INT)
-        return -6;
-      if (code == CONST_DOUBLE)
-        return -5;
-      if (code == CONST_FIXED)
-        return -5;
-      return -4;
-
-    case RTX_EXTRA:
-      /* SUBREGs of objects should come second.  */
-      if (code == SUBREG && OBJECT_P (SUBREG_REG (op)))
-        return -3;
-      return 0;
+    value = -8;
+  else if (code == CONST_DOUBLE)
+    value = -7;
+  else if (code == CONST_FIXED)
+    value = -7;
+  else
+    {
+      op = avoid_constant_pool_reference (op);
+      code = GET_CODE (op);
+
+      switch (GET_RTX_CLASS (code))
+	{
+	case RTX_CONST_OBJ:
+	  if (code == CONST_INT)
+	    value = -6;
+	  else if (code == CONST_DOUBLE)
+	    value = -5;
+	  else if (code == CONST_FIXED)
+	    value = -5;
+	  else
+	    value = -4;
+	  break;
+
+	case RTX_EXTRA:
+	  /* SUBREGs of objects should come second.  */
+	  if (code == SUBREG && OBJECT_P (SUBREG_REG (op)))
+	    value = -3;
+	  else
+	    value = 0;
+	  break;
+
+	case RTX_OBJ:
+	  /* Complex expressions should be the first, so decrease priority
+	     of objects.  */
+	  value = -1;
+	  break;
 
-    case RTX_OBJ:
-      /* Complex expressions should be the first, so decrease priority
-         of objects.  Prefer pointer objects over non pointer objects.  */
-      if ((REG_P (op) && REG_POINTER (op))
-	  || (MEM_P (op) && MEM_POINTER (op)))
-	return -1;
-      return -2;
-
-    case RTX_COMM_ARITH:
-      /* Prefer operands that are themselves commutative to be first.
-         This helps to make things linear.  In particular,
-         (and (and (reg) (reg)) (not (reg))) is canonical.  */
-      return 4;
-
-    case RTX_BIN_ARITH:
-      /* If only one operand is a binary expression, it will be the first
-         operand.  In particular,  (plus (minus (reg) (reg)) (neg (reg)))
-         is canonical, although it will usually be further simplified.  */
-      return 2;
+	case RTX_COMM_ARITH:
+	  /* Prefer operands that are themselves commutative to be first.
+	     This helps to make things linear.  In particular,
+	     (and (and (reg) (reg)) (not (reg))) is canonical.  */
+	  value = 4;
+	  break;
+
+	case RTX_BIN_ARITH:
+	  /* If only one operand is a binary expression, it will be the first
+	     operand.  In particular,  (plus (minus (reg) (reg)) (neg (reg)))
+	     is canonical, although it will usually be further simplified.  */
+	  value = 2;
+	  break;
   
-    case RTX_UNARY:
-      /* Then prefer NEG and NOT.  */
-      if (code == NEG || code == NOT)
-        return 1;
+	case RTX_UNARY:
+	  /* Then prefer NEG and NOT.  */
+	  if (code == NEG || code == NOT)
+	    value = 1;
+	  else
+	    value = 0;
+	  break;
 
-    default:
-      return 0;
+	default:
+	  value = 0;
+	}
     }
+
+  if (targetm.commutative_operand_precedence)
+    value = targetm.commutative_operand_precedence (op, value);
+
+  return value;
 }
 
 /* Return 1 iff it is necessary to swap operands of commutative operation
Index: config/rs6000/rs6000.c
===================================================================
--- config/rs6000/rs6000.c	(revision 148831)
+++ config/rs6000/rs6000.c	(working copy)
@@ -912,6 +912,7 @@ static rtx generate_set_vrsave (rtx, rs6
 int easy_vector_constant (rtx, enum machine_mode);
 static rtx rs6000_dwarf_register_span (rtx);
 static void rs6000_init_dwarf_reg_sizes_extra (tree);
+static int rs6000_commutative_operand_precedence (const_rtx, int);
 static rtx rs6000_legitimize_address (rtx, rtx, enum machine_mode);
 static rtx rs6000_legitimize_tls_address (rtx, enum tls_model);
 static void rs6000_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
@@ -1115,6 +1116,10 @@ static const struct attribute_spec rs600
 #undef TARGET_ASM_FUNCTION_EPILOGUE
 #define TARGET_ASM_FUNCTION_EPILOGUE rs6000_output_function_epilogue
 
+#undef TARGET_COMMUTATIVE_OPERAND_PRECEDENCE
+#define TARGET_COMMUTATIVE_OPERAND_PRECEDENCE \
+  rs6000_commutative_operand_precedence
+
 #undef TARGET_LEGITIMIZE_ADDRESS
 #define TARGET_LEGITIMIZE_ADDRESS rs6000_legitimize_address
 
@@ -22240,6 +22245,27 @@ rs6000_memory_move_cost (enum machine_mo
     return 4 + rs6000_register_move_cost (mode, rclass, GENERAL_REGS);
 }
 
+/* Return a value indicating whether OP, an operand of a commutative
+   operation, is preferred as the first or second operand.  The higher
+   the value, the stronger the preference for being the first operand.
+   We use negative values to indicate a preference for the first operand
+   and positive values for the second operand.
+   VALUE is the default precedence for OP; see rtlanal.c:
+   commutative_operand_precendece.  */
+
+static int
+rs6000_commutative_operand_precedence (const_rtx op, int value)
+{
+  /* Prefer pointer objects over non pointer objects.
+     For rationale see PR28690.  */
+  if (GET_RTX_CLASS (GET_CODE (op)) == RTX_OBJ
+      && ((REG_P (op) && REG_POINTER (op))
+	  || (MEM_P (op) && MEM_POINTER (op))))
+    --value;
+
+  return value;
+}
+
 /* Returns a code for a target-specific builtin that implements
    reciprocal of the function, or NULL_TREE if not available.  */