Denis Chertykov wrote:
> 2011/7/8 Georg-Johann Lay <[email protected]>:
>> 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.
Reran testsuite against newer version (175991).
First the good news.
Following tests pass, that's the reason for the patch:
* gcc.target/avr/pr46779-1.c
* gcc.target/avr/pr46779-2.c
These tests now pass, too. They all came up with spill fail both with
and without original patch, but pass with (1) and (2) added:
* gcc.c-torture/execute/pr38051.c (-Os)
* gcc.dg/20030324-1.c (-O -fstrict-aliasing -fgcse)
* gcc.dg/pr43670.c (-O -ftree-vrp -fcompare-debug)
And here the not-so-good news. There's additional ICE in reload:
* gcc.dg/pr32912-2.c (-Os)
pr32912-2.c:23:1: internal compiler error: in find_valid_class, at
reload.c:708
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
compiler exited with status 1
output is:
pr32912-2.c: In function 'bar':
pr32912-2.c:23:1: internal compiler error: in find_valid_class, at
reload.c:708
But look at the source!
#if(__SIZEOF_INT__ >= 4)
typedef int __m128i __attribute__ ((__vector_size__ (16)));
#else
typedef long __m128i __attribute__ ((__vector_size__ (16)));
#endif
That's no sensible on AVR at all!
The stack trace:
Breakpoint 1, fancy_abort (file=0x8896f38
"../../../gcc.gnu.org/trunk/gcc/reload.c", line=708,
function=0x8896fa8 "find_valid_class") at
../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893
(gdb) bt
#0 fancy_abort (file=0x8896f38
"../../../gcc.gnu.org/trunk/gcc/reload.c", line=708,
function=0x8896fa8 "find_valid_class") at
../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893
#1 0x0845a9d4 in find_valid_class (outer=SImode, inner=TImode, n=12,
dest_regno=16) at ../../../gcc.gnu.org/trunk/gcc/reload.c:708
#2 0x0845bfd5 in push_reload (in=0x0, out=0xb7dfe2c4, inloc=0x0,
outloc=0xb7dfe2d4, rclass=GENERAL_REGS, inmode=VOIDmode,
outmode=SImode, strict_low=0, optional=0, opnum=0,
type=RELOAD_FOR_OUTPUT) at ../../../gcc.gnu.org/trunk/gcc/reload.c:1196
#3 0x08462d52 in find_reloads (insn=0xb7dff144, replace=0,
ind_levels=0, live_known=1, reload_reg_p=0x89607c0) at
../../../gcc.gnu.org/trunk/gcc/reload.c:4015
#4 0x08470af9 in calculate_needs_all_insns (global=1) at
../../../gcc.gnu.org/trunk/gcc/reload1.c:1525
Note the TImode, this ICE is no harm for AVR!
As gcc.dg/pr32912-2.c is completely irrelevant for AVR,
here is the patch for review that integrates (1) and (2).
Ok?
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.
(avr_legitimate_address_p): Don't allow SUBREGs.
Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c (revision 175991)
+++ config/avr/avr.c (working copy)
@@ -1109,8 +1109,7 @@ avr_legitimate_address_p (enum machine_m
true_regnum (XEXP (x, 0)));
debug_rtx (x);
}
- if (!strict && GET_CODE (x) == SUBREG)
- x = SUBREG_REG (x);
+
if (REG_P (x) && (strict ? REG_OK_FOR_BASE_STRICT_P (x)
: REG_OK_FOR_BASE_NOSTRICT_P (x)))
r = POINTER_REGS;
@@ -6118,26 +6117,30 @@ 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)
+ /* FIXME: Ideally, the following test is not needed.
+ However, it turned out that it can reduce the number
+ of spill fails. AVR and it's poor endowment with
+ address registers is extreme stress test for reload. */
+
+ if (GET_MODE_SIZE (mode) >= 4
+ && regno >= REG_X)
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 +6413,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 +6440,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;
}
