Re: [Patch, AVR]: Fix PR46779

[Denis Chertykov <chertykov@gmail.com>]

2011/7/8 Georg-Johann Lay <avr@gjlay.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?

I think that AVR is a stress test for GCC core. We are on the edge.
IMHO your patch is a change one tweaks to another.
It's not needed if it adds regressions.

Denis.