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.
