Re: [PATCHv2] Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)

[Richard Biener <rguenther@suse.de>]

On Sun, 10 Mar 2019, Bernd Edlinger wrote:

> Hi,
> 
> This patch is an update to the previous patch, which takes into account that
> the middle-end is not supposed to use the unaligned DI value directly which
> was passed in an unaligned stack slot due to the AAPCS parameter passing rules.
> 
> The patch works by changing use_register_for_decl to return false if the
> incoming RTL is not sufficiently aligned on a STRICT_ALIGNMENT target,
> as if the address of the parameter was taken (which is TREE_ADDRESSABLE).
> So not taking the address of the parameter is a necessary condition
> for the wrong-code in PR 89544.
> 
> It works together with this check in assign_parm_adjust_stack_rtl:
>   /* If we can't trust the parm stack slot to be aligned enough for its
>      ultimate type, don't use that slot after entry.  We'll make another
>      stack slot, if we need one.  */
>   if (stack_parm
>       && ((STRICT_ALIGNMENT
>            && GET_MODE_ALIGNMENT (data->nominal_mode) > MEM_ALIGN (stack_parm))
> ...
>     stack_param = NULL
> 
> This makes assign_parms use assign_parm_setup_stack instead of
> assign_parm_setup_reg, and the latter does assign a suitably aligned
> stack slot, because stack_param == NULL by now, and uses emit_block_move
> which avoids the issue with the back-end.
> 
> Additionally, to prevent unnecessary performance regressions,
> assign_parm_find_stack_rtl is enhanced to make use of a possible larger
> alignment if the parameter was passed in an aligned stack slot without
> the ABI requiring it.
> 
> 
> Bootstrapped and reg-tested with all languages on arm-linux-gnueabihf.
> Is it OK for trunk?

I think the assign_parm_find_stack_rtl is not appropriate at this stage,
I am also missing an update to the comment of the block you change.
It also changes code I'm not familar enough with to review...

Finally...

Index: gcc/function.c
===================================================================
--- gcc/function.c      (revision 269264)
+++ gcc/function.c      (working copy)
@@ -2210,6 +2210,12 @@ use_register_for_decl (const_tree decl)
   if (DECL_MODE (decl) == BLKmode)
     return false;

+  if (STRICT_ALIGNMENT && TREE_CODE (decl) == PARM_DECL
+      && DECL_INCOMING_RTL (decl) && MEM_P (DECL_INCOMING_RTL (decl))
+      && GET_MODE_ALIGNMENT (DECL_MODE (decl))
+        > MEM_ALIGN (DECL_INCOMING_RTL (decl)))
+    return false;
+
   /* If -ffloat-store specified, don't put explicit float variables
      into registers.  */
   /* ??? This should be checked after DECL_ARTIFICIAL, but tree-ssa

I wonder if it is necessary to look at DECL_INCOMING_RTL here
and why such RTL may not exist?  That is, iff DECL_INCOMING_RTL
doesn't exist then shouldn't we return false for safety reasons?

Similarly the very same issue should exist on x86_64 which is
!STRICT_ALIGNMENT, it's just the ABI seems to provide the appropriate
alignment on the caller side.  So the STRICT_ALIGNMENT check is
a wrong one.

Which makes me think that a proper fix is not here, but in
target(hook) code.

Changing use_register_for_decl sounds odd anyways since if we return true
we for the testcase still end up in memory, no?

The hunk obviously misses a comment since the effect that this
will cause a copy to be emitted isn't obvious (and relying on
this probably fragile).

Richard.