Re: [Patch, AVR]: Fix PR46779

[Georg-Johann Lay <avr at gjlay dot de>]

CCed Eric and Bernd.

Denis Chertykov wrote:
>> Did you decide about the fix for PR46779?
>>
>> http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00810.html
>>
>> Is it ok to commit?
> 
> I forgot about testsuite regressions for this patch.
> 
> Denis.


There were no new regressions:
  http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00747.html

However, with the actual trunk (SVN 175991), I get two more
spill fails for following sources:

./gcc.c-torture/compile/pr32349.c -O1 -mmcu=atmega128

 pr30338.c: In function 'testload_func':
pr30338.c:13:1: error: unable to find a register to spill in class
'POINTER_REGS'
pr30338.c:13:1: error: this is the insn:
(insn 14 13 15 2 (set (reg:QI 24 r24 [orig:73 *D.1963_37 ] [73])
        (mem:QI (subreg:HI (reg:SI 71) 0) [0 *D.1963_37+0 S1 A8]))
pr30338.c:9 4 {*movqi}
     (expr_list:REG_DEAD (reg:SI 71)
        (nil)))
pr30338.c:13:1: internal compiler error: in spill_failure, at
reload1.c:2120



./gcc.c-torture/compile/pr32349.c -S -O3 -funroll-loops

pr32349.c: In function 'foo':
pr32349.c:26:1: error: unable to find a register to spill in class
'POINTER_REGS'
pr32349.c:26:1: error: this is the insn:
(insn 175 197 177 10 (set (reg/v:SI 234 [ m ])
        (mem:SI (post_inc:HI (reg:HI 16 r16 [orig:192 ivtmp.18 ]
[192])) [3 MEM[base: D.1996_74, offset: 0B]+0 S4 A8])) pr32349.c:18 12
{*movsi}
     (expr_list:REG_INC (reg:HI 16 r16 [orig:192 ivtmp.18 ] [192])
        (nil)))
pr32349.c:26:1: internal compiler error: in spill_failure, at
reload1.c:2120


(1)
I can fix *both* fails with additional test in avr_hard_regno_mode_ok:

+   if (GET_MODE_SIZE (mode) >= 4
+       && regno >= REG_X)
+     return 0;

(2)
I can fix the first fail but *not* the second by not allow SUBREGs in
avr_legitimate_address_p:

-   if (!strict && GET_CODE (x) == SUBREG) */
- 	x = SUBREG_REG (x); */


(2) Looks very reasonble, Eric Botcazou proposed it because he ran
into problems:
   http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01367.html

(1) Appears to be hackish, but it should be ok.  If code breaks
because of that is's *definitely* a reload bug (e.g. SI-subreg of DI).

Even the original avr_hard_regno_mode_ok is ok IMO because if a
machine says "I can hold HI in 28 but not QI in 29" reload has to
handle it (except a machine must allow word_mode in *all* it's
GENERAL_REGS, don't know if that's a must).

I made a patch for reload, too:
   http://gcc.gnu.org/ml/gcc/2011-06/msg00005.html

Because IRA generates SUBREG of hardreg (which old lreg/greg handled
ok) and reload does not handle it correctly.  It generates a spill but
without the needed input reload so that one part of the register is
missing.

reload blames IRA or BE, IRA blames reload, BE blames IRA, etc...


I didn't rerun the testsuite with (1) or/and (2), I'd like both (1)
and (2) in the compiler.  What do you think?

For reference, I attached the patch again.  It's like the original
patch, just with some comment change.

Johann


	PR target/46779
	* config/avr/avr.c (avr_hard_regno_mode_ok): Rewrite.
	In particular, allow 8-bit values in r28 and r29.
	(avr_hard_regno_scratch_ok): Disallow any register that might be
	part of the frame pointer.
	(avr_hard_regno_rename_ok): Same.
	

Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 175991)
+++ config/avr/avr.c	(working copy)
@@ -6118,26 +6118,21 @@ jump_over_one_insn_p (rtx insn, rtx dest
 int
 avr_hard_regno_mode_ok (int regno, enum machine_mode mode)
 {
-  /* Disallow QImode in stack pointer regs.  */
-  if ((regno == REG_SP || regno == (REG_SP + 1)) && mode == QImode)
-    return 0;
-
-  /* The only thing that can go into registers r28:r29 is a Pmode.  */
-  if (regno == REG_Y && mode == Pmode)
-    return 1;
-
-  /* Otherwise disallow all regno/mode combinations that span r28:r29.  */
-  if (regno <= (REG_Y + 1) && (regno + GET_MODE_SIZE (mode)) >= (REG_Y + 1))
-    return 0;
-
-  if (mode == QImode)
+  /* NOTE: 8-bit values must not be disallowed for R28 or R29.
+        Disallowing QI et al. in these regs might lead to code like
+            (set (subreg:QI (reg:HI 28) n) ...)
+        which will result in wrong code because reload does not
+        handle SUBREGs of hard regsisters like this.
+        This could be fixed in reload.  However, it appears
+        that fixing reload is not wanted by reload people.  */
+  
+  /* Any GENERAL_REGS register can hold 8-bit values.  */
+  
+  if (GET_MODE_SIZE (mode) == 1)
     return 1;
-
-  /* Modes larger than QImode occupy consecutive registers.  */
-  if (regno + GET_MODE_SIZE (mode) > FIRST_PSEUDO_REGISTER)
-    return 0;
-
-  /* All modes larger than QImode should start in an even register.  */
+  
+  /* All modes larger than 8 bits should start in an even register.  */
+  
   return !(regno & 1);
 }
 
@@ -6410,13 +6405,23 @@ avr_hard_regno_scratch_ok (unsigned int
       && !df_regs_ever_live_p (regno))
     return false;
 
+  /* Don't allow hard registers that might be part of the frame pointer.
+     Some places in the compiler just test for [HARD_]FRAME_POINTER_REGNUM
+     and don't care for a frame pointer that spans more than one register.  */
+
+  if ((!reload_completed || frame_pointer_needed)
+      && (regno == REG_Y || regno == REG_Y + 1))
+    {
+      return false;
+    }
+
   return true;
 }
 
 /* Return nonzero if register OLD_REG can be renamed to register NEW_REG.  */
 
 int
-avr_hard_regno_rename_ok (unsigned int old_reg ATTRIBUTE_UNUSED,
+avr_hard_regno_rename_ok (unsigned int old_reg,
 			  unsigned int new_reg)
 {
   /* Interrupt functions can only use registers that have already been
@@ -6427,6 +6432,17 @@ avr_hard_regno_rename_ok (unsigned int o
       && !df_regs_ever_live_p (new_reg))
     return 0;
 
+  /* Don't allow hard registers that might be part of the frame pointer.
+     Some places in the compiler just test for [HARD_]FRAME_POINTER_REGNUM
+     and don't care for a frame pointer that spans more than one register.  */
+
+  if ((!reload_completed || frame_pointer_needed)
+      && (old_reg == REG_Y || old_reg == REG_Y + 1
+          || new_reg == REG_Y || new_reg == REG_Y + 1))
+    {
+      return 0;
+    }
+  
   return 1;
 }