PATCH RFC: Fix ARM bug by splitting up iwmmxt_movsi_insn

[Ian Lance Taylor <ian at wasabisystems dot com>]

This simple C++ program breaks the mainline C++ compiler when compiled
with
    -mcpu=iwmmxt -O2

inline const int&
min(const int& __a, const int& __b)
{ if (__b < __a) return __b; return __a; }
int foo (const char* f, const char* e, int max)
{ return min(max, e - f); }

The result is a crash with "could not split insn".

This happens because the ifcompare_plus_move insn requires a split,
but the condition for the corresponding define_split has
!TARGET_IWMMXT, with this comment:
    ;; Note we have to suppress this split for the iwmmxt because it
    ;; creates a conditional movsi and the iwmmxt_movsi_insn pattern
    ;; is not predicable.  This sucks.

I was unable to find a simple C test case, although I didn't look too
hard.  Note that even minor changes to the C++ test case cause it to
compile successfully.

The simple way out of this is to change ifcompare_plus_move and
friends to have a condition of !TARGET_IWMMXT.

However, it seems to me that the basic issue here may be wrong.
Currently arm_movsi_insn has a condition of !TARGET_IWMMXT, and on the
IWMMXT all moves are handled by iwmmxt_movsi_insn, which can not be
predicable because (according to the comments) the alternative to
store a IWMMXT general register to memory is not predicable.

But perhaps moves for ordinary registers can not continue to be
handled by arm_movsi_insn, which is predicable, while moves for IWMMXT
registers are handled by iwmmxt_movsi_insn.

That leads to the following patch.  This patch does not introduce any
new testsuite compilation failures.  However, most ports don't seem to
work this way.  In particular, it's possible that with the original
code, in some cases, reload will sometimes choose a register in
IWMMXT_GR_REGS, whereas with the patched code, it no longer will.  I'm
not sure, and I'd like to hear any feedback which others may have.

(This patch includes a patch to arm_regno_class() which is really a
simple bug fix.)

Ian

Index: arm.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/arm/arm.c,v
retrieving revision 1.294
diff -u -p -r1.294 arm.c
--- arm.c	7 Oct 2003 08:49:35 -0000	1.294
+++ arm.c	10 Oct 2003 06:59:00 -0000
@@ -3934,6 +3934,19 @@ cirrus_shift_const (rtx op, enum machine
 	  && INTVAL (op) < 64);
 }
 
+/* Return nonzero if OP is a IWMMXT general register.  */
+int
+iwmmxt_general_register_operand (rtx op, enum machine_mode mode)
+{
+  if (GET_MODE (op) != mode && mode != VOIDmode)
+    return FALSE;
+  if (GET_CODE (op) == SUBREG)
+    op = SUBREG_REG (op);
+  return (GET_CODE (op) == REG
+	  && REGNO (op) < FIRST_PSEUDO_REGISTER
+	  && REGNO_REG_CLASS (REGNO (op)) == IWMMXT_GR_REGS);
+}
+
 /* Returns TRUE if INSN is an "LDR REG, ADDR" instruction.
    Use by the Cirrus Maverick code which has to workaround
    a hardware bug triggered by such instructions.  */
@@ -10299,6 +10312,9 @@ arm_regno_class (int regno)
 
   if (IS_IWMMXT_REGNUM (regno))
     return IWMMXT_REGS;
+
+  if (IS_IWMMXT_GR_REGNUM (regno))
+    return IWMMXT_GR_REGS;
 
   return FPA_REGS;
 }
Index: arm.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/arm/arm.h,v
retrieving revision 1.206
diff -u -p -r1.206 arm.h
--- arm.h	7 Oct 2003 08:49:36 -0000	1.206
+++ arm.h	10 Oct 2003 06:59:02 -0000
@@ -2713,6 +2713,7 @@ extern int making_const_table;
   {"cirrus_register_operand", {REG}},					\
   {"cirrus_fp_register", {REG}},					\
   {"cirrus_shift_const", {CONST_INT}},					\
+  {"iwmmxt_general_register_operand", {SUBREG, REG}},			\
   {"dominant_cc_register", {REG}},
 
 /* Define this if you have special predicates that know special things
Index: arm.md
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/arm/arm.md,v
retrieving revision 1.141
diff -u -p -r1.141 arm.md
--- arm.md	7 Oct 2003 08:49:36 -0000	1.141
+++ arm.md	10 Oct 2003 06:59:05 -0000
@@ -3759,9 +3759,11 @@
 (define_insn "*arm_movsi_insn"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r, m")
 	(match_operand:SI 1 "general_operand"      "rI,K,mi,r"))]
-  "TARGET_ARM && ! TARGET_IWMMXT
+  "TARGET_ARM
    && (   register_operand (operands[0], SImode)
-       || register_operand (operands[1], SImode))"
+       || register_operand (operands[1], SImode))
+   && ! iwmmxt_general_register_operand (operands[0], SImode)
+   && ! iwmmxt_general_register_operand (operands[1], SImode)"
   "@
    mov%?\\t%0, %1
    mvn%?\\t%0, #%B1
@@ -8522,10 +8524,7 @@
 			 (match_dup 0)
 			 (match_operand 4 "" "")))
    (clobber (reg:CC CC_REGNUM))]
-  ;; Note we have to suppress this split for the iwmmxt because it
-  ;; creates a conditional movsi and the iwmmxt_movsi_insn pattern
-  ;; is not predicable.  This sucks.
-  "TARGET_ARM && reload_completed && ! TARGET_IWMMXT"
+  "TARGET_ARM && reload_completed"
   [(set (match_dup 5) (match_dup 6))
    (cond_exec (match_dup 7)
 	      (set (match_dup 0) (match_dup 4)))]
@@ -8553,10 +8552,7 @@
 			 (match_operand 4 "" "")
 			 (match_dup 0)))
    (clobber (reg:CC CC_REGNUM))]
-  ;; Note we have to suppress this split for the iwmmxt because it
-  ;; creates a conditional movsi and the iwmmxt_movsi_insn pattern
-  ;; is not predicable.  This sucks.
-  "TARGET_ARM && reload_completed && ! TARGET_IWMMXT"
+  "TARGET_ARM && reload_completed"
   [(set (match_dup 5) (match_dup 6))
    (cond_exec (match_op_dup 1 [(match_dup 5) (const_int 0)])
 	      (set (match_dup 0) (match_dup 4)))]
@@ -8577,10 +8573,7 @@
 			 (match_operand 4 "" "")
 			 (match_operand 5 "" "")))
    (clobber (reg:CC CC_REGNUM))]
-  ;; Note we have to suppress this split for the iwmmxt because it
-  ;; creates a conditional movsi and the iwmmxt_movsi_insn pattern
-  ;; is not predicable.  This sucks.
-  "TARGET_ARM && reload_completed && ! TARGET_IWMMXT"
+  "TARGET_ARM && reload_completed"
   [(set (match_dup 6) (match_dup 7))
    (cond_exec (match_op_dup 1 [(match_dup 6) (const_int 0)])
 	      (set (match_dup 0) (match_dup 4)))
@@ -8612,10 +8605,7 @@
 			 (not:SI
 			  (match_operand:SI 5 "s_register_operand" ""))))
    (clobber (reg:CC CC_REGNUM))]
-  ;; Note we have to suppress this split for the iwmmxt because it
-  ;; creates a conditional movsi and the iwmmxt_movsi_insn pattern
-  ;; is not predicable.  This sucks.
-  "TARGET_ARM && reload_completed && ! TARGET_IWMMXT"
+  "TARGET_ARM && reload_completed"
   [(set (match_dup 6) (match_dup 7))
    (cond_exec (match_op_dup 1 [(match_dup 6) (const_int 0)])
 	      (set (match_dup 0) (match_dup 4)))
Index: iwmmxt.md
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/arm/iwmmxt.md,v
retrieving revision 1.2
diff -u -p -r1.2 iwmmxt.md
--- iwmmxt.md	28 Jun 2003 19:43:00 -0000	1.2
+++ iwmmxt.md	10 Oct 2003 06:59:06 -0000
@@ -92,28 +92,24 @@
 )
 
 (define_insn "*iwmmxt_movsi_insn"
-  [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r, m,z,r,?z,m,z")
-	(match_operand:SI 1 "general_operand"      "rI,K,mi,r,r,z,m,z,z"))]
+  [(set (match_operand:SI 0 "nonimmediate_operand" "=z,r,?z,m,z")
+	(match_operand:SI 1 "general_operand"      " r,z, m,z,z"))]
   "TARGET_REALLY_IWMMXT
-   && (   register_operand (operands[0], SImode)
-       || register_operand (operands[1], SImode))"
+   && (iwmmxt_general_register_operand (operands[0], SImode)
+       || iwmmxt_general_register_operand (operands[1], SImode))"
   "*
    switch (which_alternative)
    {
-   case 0: return \"mov\\t%0, %1\";
-   case 1: return \"mvn\\t%0, #%B1\";
-   case 2: return \"ldr\\t%0, %1\";
-   case 3: return \"str\\t%1, %0\";
-   case 4: return \"tmcr\\t%0, %1\";
-   case 5: return \"tmrc\\t%0, %1\";
-   case 6: return arm_output_load_gr (operands);
-   case 7: return \"wstrw\\t%1, %0\";
+   case 0: return \"tmcr\\t%0, %1\";
+   case 1: return \"tmrc\\t%0, %1\";
+   case 2: return arm_output_load_gr (operands);
+   case 3: return \"wstrw\\t%1, %0\";
    default:return \"wstrw\\t%1, [sp, #-4]!\;wldrw\\t%0, [sp], #4\\t@move CG reg\";
   }"
-  [(set_attr "type"           "*,*,load,store1,*,*,load,store1,*")
-   (set_attr "length"         "*,*,*,        *,*,*,  16,     *,8")
-   (set_attr "pool_range"     "*,*,4096,     *,*,*,1024,     *,*")
-   (set_attr "neg_pool_range" "*,*,4084,     *,*,*,   *,  1012,*")
+  [(set_attr "type"           "*,*,load,store1,*")
+   (set_attr "length"         "*,*,  16,     *,8")
+   (set_attr "pool_range"     "*,*,1024,     *,*")
+   (set_attr "neg_pool_range" "*,*,   *,  1012,*")
    ;; Note - the "predicable" attribute is not allowed to have alternatives.
    ;; Since the wSTRw wCx instruction is not predicable, we cannot support
    ;; predicating any of the alternatives in this template.  This sucks.