Re: [Patch, AVR]: Fix PR46779

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

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.

I am unsure about that. There is the "q" constraint that is used for
SP. register_operand will accept SP because regno_reg_class does not
return NO_REGS for SP. So something must stop the register allocator
from allocating ordinary data to SP.

In the current md there is "*movhi_sp" insn prior to "*movhi" insn.
Besides that it is strongly discouraged to have more than one move
insn for one mode, not each and every place looks at insn conditions.

Moreover, if there is an insn like
(set (reg:HI 100)
     (reg:HI 101))
that matches "*movhi", what will keep IRA from allocating one of the
values to SP, i.e. chose alternative "q,r" or "r,q"?

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

I agree with you. However, I think that the risk of spill failure
should be minimized. I have no idea how to fix a splill failure like
the following that occurs in testsuite (with -Os, no matter if the
patch is applied or not):

./gcc/testsuite/gcc.c-torture/execute/pr38051.c:189:1: error: unable
to find a register to spill in class 'POINTER_REGS'
./gcc/testsuite/gcc.c-torture/execute/pr38051.c:189:1: error: this is
the insn:
(insn 61 60 62 10 (set (reg/v:SI 12 r12 [orig:73 b0 ] [73])
        (mem:SI (subreg:HI (reg/v:SI 70 [ srcp2 ]) 0) [2 *D.2218_56+0
S4 A8])) ./gcc/testsuite/gcc.c-torture/execute/pr38051.c:64 12 {*movsi}
     (nil))
./gcc/testsuite/gcc.c-torture/execute/pr38051.c:189:1: internal
compiler error: in spill_failure, at reload1.c:2113

Actually I have no idea if the snip in avr_hard_regno_mode_ok actually
would reduce the risk of spill failure :-/

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

Johann