[Bug target/65657] [avr] read from __memx address space tramples argument to following function

[Bug target/65657] [avr] read from __memx address space tramples argument to following function

[jonathan.creekmore@synapse-wireless.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65657

--- Comment #1 from Jonathan Creekmore <jonathan.creekmore@synapse-wireless.com> ---
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.

[Bug target/65657] [avr] read from __memx address space tramples argument to following function

[cardoe at gentoo dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65657

--- Comment #2 from Doug Goldstein <cardoe at gentoo dot org> ---
I can confirm the same issue affects gcc 4.9.2

[Bug target/65657] [avr] read from __memx address space tramples argument to following function

[senthil_kumar.selvaraj at atmel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65657

Senthil Kumar Selvaraj <senthil_kumar.selvaraj at atmel dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |senthil_kumar.selvaraj@atme
                   |                            |l.com

--- Comment #3 from Senthil Kumar Selvaraj <senthil_kumar.selvaraj at atmel dot com> ---
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

[Bug target/65657] [avr] read from __memx address space tramples argument to following function

[senthil_kumar.selvaraj at atmel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65657

--- Comment #4 from Senthil Kumar Selvaraj <senthil_kumar.selvaraj at atmel dot com> ---
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)

[Bug target/65657] [avr] read from __memx address space tramples argument to following function

[senthil_kumar.selvaraj at atmel dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65657

--- Comment #5 from Senthil Kumar Selvaraj <senthil_kumar.selvaraj at atmel dot com> ---
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.

[Bug target/65657] [avr] read from __memx address space tramples argument to following function

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65657

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |avr
           Priority|P3                          |P4
             Status|UNCONFIRMED                 |RESOLVED
                 CC|                            |gjl at gcc dot gnu.org
         Resolution|---                         |DUPLICATE

--- Comment #6 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
(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 ***