Re: [patch,avr] Fix PR 65657 - read from __memx address space tramples arguments to function call

[Georg-Johann Lay <avr@gjlay.de>]

Am 04/16/2015 um 08:43 AM schrieb Senthil Kumar Selvaraj:
> This patch fixes PR 65657.

The following artifact appears to be PR63633.

> When cfgexpand.c expands a function call, it first figures out the
> registers in which the arguments will go, followed by expansion of the
> arguments themselves (right to left). It later emits mov insns to set
> the precomputed registers with the expanded RTXes.
>
> If one of the arguments is a __memx char pointer dereference, the mov
> eventually expands to gen_xload<mode>_A (for certain cases), which
> clobbers R22, R21 and Z.  This causes problems if one of those
> clobbered registers was precomputed to hold another argument.
>
> In general, call expansion does not appear to take clobbers into account -
> when it checks for argument overlap, the RTX (args[i].value) is only a MEM
> in QImode for the memx deref - the clobber shows up when it eventually
> calls emit_move_insn, at which point, it is too late.
>
> This does not happen for a int pointer dereference - 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. This is done
> before any moves to arg registers is done, so other arguments are not
> overwritten.
>
> Doing the same thing - providing a better cost estimate for a MEM rtx in
> the non-default address space, makes this problem go away, and that is
> what this patch does. Regression testing does not show any new failures.
>
> The fix does seem tangential to the core problem - not sure if there is
> a better way to fix this?
>
> If not, could someone commit this please? I don't have commit access.
>
> Regards
> Senthil
>
> gcc/ChangeLog
>
> 2015-04-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	* config/avr/avr.c (avr_rtx_costs_1): Increase cost for
> 	MEM rtx in non-default address space.
>
> gcc/testsuite/ChangeLog
>
> 2015-04-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	* gcc.target/avr/pr65657.c: New.
>
> diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
> index 68d5ddc..c9b8678 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;

IMO costs should not be used to fix wrong code or ICEs.  Presumably the fix is 
similar to the one used by PR63633.

Johann

>       case NEG:
> diff --git gcc/testsuite/gcc.target/avr/pr65657.c gcc/testsuite/gcc.target/avr/pr65657.c
> new file mode 100644
> index 0000000..d908fe3
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr65657.c
> @@ -0,0 +1,39 @@
> +/* { dg-do run } */
> +/* { dg-options "-Os" } */
> +
> +/* When cfgexpand expands the call to foo, it
> +   assigns registers R22:R23 to 0xABCD and R24
> +   to *x. Because x is a __memx address space
> +   pointer, the dereference results in an RTL
> +   insn that clobbers R22 among other
> +   registers (xload<mode>_A).
> +
> +   Call expansion does not take this into account
> +   and ends up clobbering R22 set previously to
> +   hold part of the second argument.
> +*/
> +
> +#include <stdlib.h>
> +
> +void __attribute__((noinline))
> +    __attribute__((noclone))
> +foo (char c, volatile unsigned int d)
> +{
> +    if (d != 0xABCD)
> +      abort();
> +    if (c != 'A')
> +        abort();
> +}
> +
> +void __attribute__((noinline))
> +    __attribute__((noclone))
> +readx(const char __memx *x)
> +{
> +    foo (*x, 0xABCD);
> +}
> +
> +const char __memx arr[] = { 'A' };
> +int main()
> +{
> +   readx (arr);
> +}
>