Created attachment 35207 [details] Test snippet that exhibits the problem. For an AVR target, the following code that reads from the __memx address space results in the wrong code and tramples the handle variable: extern void writeFlashByte(uint8_t byte, uint16_t handle); void writeBuf(const uint8_t __memx *from, const uint8_t __memx *to) { uint16_t handle = ((uint16_t)(((__uint24)to) & 0xFFFFUL)); writeFlashByte(*from++, handle++); } The relevant assembly code is as follows: mov r27,r22 # Move 'from' out of the way mov r26,r23 # Move 'from' out of the way mov r21,r24 # Move 'from' out of the way mov r24,r20 # Move 'to' into 'handle' for calling writeFlashByte movw r22,r18 # Move 'to' into 'handle' for calling writeFlashByte mov r30,r27 # Put the relevant bits of 'from' into place to call __xload_1 mov r31,r26 # Put the relevant bits of 'from' into place to call __xload_1 call __xload_1 # <-- Returns into r22, corrupting the lower byte of 'handle' mov r24,r22 # Move the result of __xload_1 for calling writeFlashByte jmp writeFlashByte The line marked <-- is where the badness occurs. Since __xload_1 returns into r22, the 'handle' argument to writeFlashByte (which should be in r22 and r23), gets its r22 portion corrupted by the value __xload_1. PR target/52484 mentioned adding R22 to the register footprint for xload, but that was targeted to 4.7.1, so I am not sure why this is failing in 4.8.1. == compile == avr-gcc -save-temps -mmcu=atmega128rfa1 -Os -v -Wall -Wextra -c test.c == configure == ../src/configure -v --enable-languages=c,c++ --prefix=/usr/lib --infodir=/usr/share/info --mandir=/usr/share/man --bindir=/usr/bin --libexecdir=/usr/lib --libdir=/usr/lib --enable-shared --with-system-zlib --enable-long-long --enable-nls --without-included-gettext --disable-libssp --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=avr Thread model: single gcc version 4.8.1 (GCC)
Apparently, PR target/52484 covered one of the cases in avr.md, but not the general case of a call to __xload_{1,2,3}. I think adding (clobber (reg:MOVMODE 22)) there as well might fix it.
I can confirm the same issue affects gcc 4.9.2
Happens on a recent trunk build as well. Here's a simpler testcase. $ cat foo.c void foo (char a, unsigned int b); void readx (const char __memx *x) { foo (*x, 0xABCD); } $ avr-gcc -mmcu=atmega1280 foo.c -S -Os $ cat foo.s <snip> mov r18,r22 mov r25,r23 ldi r22,lo8(-51); Load 0xABCD into r22:r23 in prep for call to foo ldi r23,lo8(-85) mov r30,r18 mov r31,r25 mov r21,r24 call __xload_1; r22 clobbered here mov r24,r22 jmp foo
Doesn't appear to be a missed clobber in the md file, as *.expand shows in insn 7 - r22 is in the clobbered registers list. Later passes assume r22 is unused after insn 6 (reg:R22 QI is marked as REG_UNUSED), and proceed to set r22 instead of r24 in insn 7. (note 4 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) (insn 2 4 3 2 (set (reg/v/f:PSI 43 [ x ]) (reg:PSI 22 r22 [ x ])) foo.c:3 -1 (nil)) (note 3 2 6 2 NOTE_INSN_FUNCTION_BEG) (insn 6 3 7 2 (set (reg:HI 22 r22) (const_int -21555 [0xffffffffffffabcd])) foo.c:4 -1 (nil)) (insn 7 6 8 2 (parallel [ (set (reg:QI 24 r24) (mem:QI (reg/v/f:PSI 43 [ x ]) [0 *x_2(D)+0 S1 A8 AS7])) (clobber (reg:QI 22 r22)) (clobber (reg:QI 21 r21)) (clobber (reg:HI 30 r30)) ]) foo.c:4 -1 (nil)) (call_insn/j 8 7 9 2 (parallel [ (call (mem:HI (symbol_ref:HI ("foo") [flags 0x41] <function_decl 0x7f635059f360 foo>) [0 foo S2 A8]) (const_int 0 [0])) (use (const_int 1 [0x1])) ]) foo.c:4 -1 (expr_list:REG_CALL_DECL (symbol_ref:HI ("foo") [flags 0x41] <function_decl 0x7f635059f360 foo>) (nil)) (expr_list:QI (use (reg:QI 24 r24)) (expr_list:HI (use (reg:HI 22 r22)) (nil)))) (barrier 9 8 0)
This tentative patch (pending regression tests) makes the problem go away diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c index 68d5ddc..46ff7e1 100644 --- gcc/config/avr/avr.c +++ gcc/config/avr/avr.c @@ -9959,7 +9959,11 @@ avr_rtx_costs_1 (rtx x, int codearg, int outer_code ATTRIBUTE_UNUSED, return true; case MEM: - *total = COSTS_N_INSNS (GET_MODE_SIZE (mode)); + /* MEM rtx with non-default address space is more + expensive. Not expressing that results in reg + clobber during expand (PR 65657). */ + *total = COSTS_N_INSNS (GET_MODE_SIZE (mode) + + (MEM_ADDR_SPACE(x) == ADDR_SPACE_RAM ? 0 : 5)); return true; case NEG: Function call arguments are expanded right to left, which means that when expanding the call to foo, R22:R23 is set to 0xABCD first up, and then the expansion of *x clobbers R22 when mov<mode> calls gen_xload<mode>_A. Call expansion does not appear to take the clobber (reg:MOVMODE 22) into account - when it checks for argument overlap, the RTL (args[i].value) is only a MEM in QImode - the clobber shows up when it eventually calls emit_move_insn. This does not happen if x is a int pointer (rather than char) - turns out that precompute_register_parameters does a copy_to_mode_reg if the cost of args[i].value is more than COSTS_N_INSNS(1) i.e., it creates a pseudo and later assigns the pseudo to the arg register. Doing the same thing - providing a better cost estimate for a MEM rtx in the non-default address space, makes this problem go away.
(In reply to Senthil Kumar Selvaraj from comment #5) > This tentative patch (pending regression tests) makes the problem go away > [...] > @@ -9959,7 +9959,11 @@ avr_rtx_costs_1 (rtx x, int codearg, int outer_code > ATTRIBUTE_UNUSED, > return true; > > case MEM: > - *total = COSTS_N_INSNS (GET_MODE_SIZE (mode)); > + /* MEM rtx with non-default address space is more > + expensive. Not expressing that results in reg > + clobber during expand (PR 65657). */ > + *total = COSTS_N_INSNS (GET_MODE_SIZE (mode) > + + (MEM_ADDR_SPACE(x) == ADDR_SPACE_RAM ? 0 : 5)); This might lead to better code, but costs should never be a proper fix for wrong code or ICE. *** This bug has been marked as a duplicate of bug 63633 ***
*** This bug has been marked as a duplicate of bug 87376 ***
Also duplicate of PR86635, aleady assigned to Senthil. *** This bug has been marked as a duplicate of bug 86635 ***
(In reply to Georg-Johann Lay from comment #6) > > *** This bug has been marked as a duplicate of bug 63633 *** (In reply to Georg-Johann Lay from comment #7) > > *** This bug has been marked as a duplicate of bug 87376 *** (In reply to Georg-Johann Lay from comment #8) > Also duplicate of PR86635, aleady assigned to Senthil. > > *** This bug has been marked as a duplicate of bug 86635 *** Since changing which bug this is a duplicate of overwrites the previous value, be sure to put the previous duplicates under "See Also" or something.