diff mbox

[AVR] : Fix PR46779

Message ID 4DF1DF4E.3030800@gjlay.de
State New
Headers show

Commit Message

Georg-Johann Lay June 10, 2011, 9:09 a.m. UTC 
Denis Chertykov schrieb:
> 2011/6/9 Georg-Johann Lay <avr@gjlay.de>:
>> This is a tentative patch to fix PR46779 and hopefully also related
>> issues like PR45291.
>>
> -  /* Disallow QImode in stack pointer regs.  */
> -  if ((regno == REG_SP || regno == (REG_SP + 1)) && mode == QImode)
> +  /* Don't allocate data to non-GENERAL_REGS registers.  */
> +
> +  if (regno >= 32)
>      return 0;
> 
> I think that we not need in this code.
> GCC core must bother about this.
> 
> +
> +  if (GET_MODE_SIZE (mode) == 1)
>      return 1;
> 
> I'm agree with this.
> 
> +
> +  /* Disallow big registers that overlap the frame pointer.
> +     This will hopefully reduce the number of spill failures.  */
> +
> +  if (GET_MODE_SIZE (mode) > 2
> +      && regno <= REG_Y
> +      && regno + GET_MODE_SIZE (mode) >= REG_Y + 1)
> +    {
> +      return 0;
> +    }
> 
> Fragment from GCC info:
> --------------------------------------
> HARD_REGNO_MODE_OK (regno, mode)A C expression that is nonzero if it
> is permissible to store a value of mode mode in hard register number
> regno (or in several registers starting with that one). For a machine
> where all registers are equivalent, a suitable definition is
> 
> #define HARD_REGNO_MODE_OK(REGNO, MODE) 1
> 
> You need not include code to check for the numbers of fixed registers,
> because the allocation mechanism considers them to be always occupied.
> -----------------------------------------
> Again, GCC core must bother about this.
> 
> -  /* 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)
> -    return 1;
> -
> -  /* Modes larger than QImode occupy consecutive registers.  */
> -  if (regno + GET_MODE_SIZE (mode) > FIRST_PSEUDO_REGISTER)
> -    return 0;
> -
> 
> This is a right change.
> 
> Denis.

So the patch turns avr_hard_regno_mode_ok into plain vanilla...

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.

Comments

Georg-Johann Lay June 21, 2011, 4:43 p.m. UTC | #1 
Do you think we can split the patch intended to fix PR46779

> 	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.

http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00810.html

from the other work discussed in this thread that aims at fixing
PR45291 which is "just" an optimization issue because of fake X
addressing?

These PR are two different things.

Johann
diff mbox

Patch

Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(Revision 174701)
+++ config/avr/avr.c	(Arbeitskopie)
@@ -6276,26 +6276,20 @@  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)
+  /* Any GENERAL_REGS register can hold 8-bit values.  */
+  /* FIXME:
+     8-bit values must not be disallowed for R28 or R29.  Disallowing
+     QI et al. in these registers might lead to code like
+         (set (subreg:QI (reg:HI 28)) ...)
+     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.  */
+  
+  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);
 }
 
@@ -6422,13 +6416,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
@@ -6439,6 +6443,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;
 }