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; }