Denis Chertykov wrote: > 2011/7/8 Georg-Johann Lay : >> 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 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.