Re: [Patch, AVR]: Fix PR49487 (ICE for wrong rotate scratch)

[Georg-Johann Lay <avr@gjlay.de>]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

http://gcc.gnu.org/ml/gcc-patches/2011-07/msg01036.html

Georg-Johann Lay wrote:
> This is a patch to fix PR49487.

Forgot to attach it. Here it is.

> As Denis will be off-line for some time, it'd be great if
> a global reviewer would review it.  It appears that he is
> the only AVR maintainer who approves patches.
> 
> The reason for the ICE is as explained in the PR:
> 
> Rotate pattern use "X" as constraint for an operand which is
> used as scratch.  However, the operand is a match_operand
> and not a match_scratch.
> 
> Because the scratch is not needed in all situations, I choose
> to use match_scratch instead of match_operand and not to fix
> the constraints.  Fixing constraints would lead to superfluous
> allocation of register if no scratch was needed.
> 
> Tested with 2 FAILs less: gcc.c-torture/compile/pr46883.c
> ICEs without this patch and passes with it.
> 
> The test case in the PR passes, too. That test case
> passes also the current unpatched 4.7, but it's obvious that
> the constraint/operand combination is a bug.
> 
> Ok to commit and back-port to 4.6?
> 
> Johann
> 
> 	PR target/49487
> 	* config/avr/avr.md (rotl<mode>3): Generate SCRATCH instead
> 	of REG.
> 	(*rotw<mode>): Use const_int_operand for operands2.
> 	Use match_scatch for operands3.
> 	(*rotb<mode>): Ditto
> 	* config/avr/avr.c (avr_rotate_bytes): Treat SCRATCH.



[-- Attachment #2: pr49487.diff --]
[-- Type: text/x-patch, Size: 3988 bytes --]

Index: config/avr/avr.md
===================================================================
--- config/avr/avr.md	(revision 176136)
+++ config/avr/avr.md	(working copy)
@@ -1597,18 +1597,18 @@ (define_expand "rotl<mode>3"
 				(match_operand:VOID 2 "const_int_operand" "")))
 		(clobber (match_dup 3))])]
   ""
-  "
-{
-  if (CONST_INT_P (operands[2]) && 0 == (INTVAL (operands[2]) % 8))
   {
-  if (AVR_HAVE_MOVW && 0 == INTVAL (operands[2]) % 16)
-    operands[3] = gen_reg_rtx (<rotsmode>mode);
-  else
-    operands[3] = gen_reg_rtx (QImode);
-  }
-  else
-    FAIL;
-}")
+    if (CONST_INT_P (operands[2])
+        && 0 == INTVAL (operands[2]) % 8)
+      {
+        if (AVR_HAVE_MOVW && 0 == INTVAL (operands[2]) % 16)
+          operands[3] = gen_rtx_SCRATCH (<rotsmode>mode);
+        else
+          operands[3] = gen_rtx_SCRATCH (QImode);
+      }
+    else
+      FAIL;
+  })
 
 
 ;; Overlapping non-HImode registers often (but not always) need a scratch.
@@ -1620,34 +1620,38 @@ (define_expand "rotl<mode>3"
 ; Split word aligned rotates using scratch that is mode dependent.
 (define_insn_and_split "*rotw<mode>"
   [(set (match_operand:HIDI 0 "register_operand" "=r,r,#&r")
-	(rotate:HIDI (match_operand:HIDI 1 "register_operand" "0,r,r")
-		     (match_operand 2 "immediate_operand" "n,n,n")))
-   (clobber (match_operand:<rotsmode> 3 "register_operand"  "=<rotx>" ))]
-  "(CONST_INT_P (operands[2]) &&
-     (0 == (INTVAL (operands[2]) % 16) && AVR_HAVE_MOVW))"
+        (rotate:HIDI (match_operand:HIDI 1 "register_operand" "0,r,r")
+                     (match_operand 2 "const_int_operand" "n,n,n")))
+   (clobber (match_scratch:<rotsmode> 3 "=<rotx>"))]
+  "AVR_HAVE_MOVW
+   && CONST_INT_P (operands[2])
+   && 0 == INTVAL (operands[2]) % 16"
   "#"
   "&& (reload_completed || <MODE>mode == DImode)"
   [(const_int 0)]
-  "avr_rotate_bytes (operands);
-  DONE;"
-)
+  {
+    avr_rotate_bytes (operands);
+    DONE;
+  })
 
 
 ; Split byte aligned rotates using scratch that is always QI mode.
 (define_insn_and_split "*rotb<mode>"
   [(set (match_operand:HIDI 0 "register_operand" "=r,r,#&r")
-	(rotate:HIDI (match_operand:HIDI 1 "register_operand" "0,r,r")
-		     (match_operand 2 "immediate_operand" "n,n,n")))
-   (clobber (match_operand:QI 3 "register_operand" "=<rotx>" ))]
-  "(CONST_INT_P (operands[2]) &&
-     (8 == (INTVAL (operands[2]) % 16)
-     	|| (!AVR_HAVE_MOVW && 0 == (INTVAL (operands[2]) % 16))))"
+        (rotate:HIDI (match_operand:HIDI 1 "register_operand" "0,r,r")
+                     (match_operand 2 "const_int_operand" "n,n,n")))
+   (clobber (match_scratch:QI 3 "=<rotx>"))]
+  "CONST_INT_P (operands[2])
+   && (8 == INTVAL (operands[2]) % 16
+       || (!AVR_HAVE_MOVW
+           && 0 == INTVAL (operands[2]) % 16))"
   "#"
   "&& (reload_completed || <MODE>mode == DImode)"
   [(const_int 0)]
-  "avr_rotate_bytes (operands);
-  DONE;"
-)
+  {
+    avr_rotate_bytes (operands);
+    DONE;
+  })
 
 
 ;;<< << << << << << << << << << << << << << << << << << << << << << << << << <<
Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 176141)
+++ config/avr/avr.c	(working copy)
@@ -4438,7 +4438,9 @@ avr_rotate_bytes (rtx operands[])
     if (mode == DImode)
       move_mode = QImode;
     /* Make scratch smaller if needed.  */
-    if (GET_MODE (scratch) == HImode && move_mode == QImode)
+    if (SCRATCH != GET_CODE (scratch)
+        && HImode == GET_MODE (scratch)
+        && QImode == move_mode)
       scratch = simplify_gen_subreg (move_mode, scratch, HImode, 0); 
 
     move_size = GET_MODE_SIZE (move_mode);
@@ -4534,6 +4536,8 @@ avr_rotate_bytes (rtx operands[])
 		   When this move occurs, it will break chain deadlock.
 		   The scratch register is substituted for real move.  */
 
+		gcc_assert (SCRATCH != GET_CODE (scratch));
+
 		move[size].src = move[blocked].dst;
 		move[size].dst =  scratch;
 		/* Scratch move is never blocked.  */