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

[Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>]

On Thu, Apr 16, 2015 at 11:02:05AM +0200, Georg-Johann Lay wrote:
> Am 04/16/2015 um 08:43 AM schrieb Senthil Kumar Selvaraj:
> >This patch fixes PR 65657.
> 
> The following artifact appears to be PR63633.
> 

I did see that one - unfortunately, that fix won't help here. IIUC, you
check if input/output operand hard regs are in the clobber list, 
and if yes, you generate pseudos to save and restore clobbered hard
regs.

In this case, the reg is actually clobbered by a different insn (one
that loads the next argument to the function). So unless I blindly generate pseudos for 
all regs in the clobber list, the clobbering will still happen.

FWIW, I did try saving/restoring all clobbered regs, and it did fix the 
problem - just that it appeared like a (worse) hack to me. Aren't we
manually replicating what RA/reload should be doing?

What do you think?

> >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.

Yes, fixing it by adjusting costs is fragile, but it appeared cleaner to me.

Regards
Senthil

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