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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 3/21/19 12:15 PM, Richard Biener wrote:
> On Sun, 10 Mar 2019, Bernd Edlinger wrote:
> 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?
> 

I think that happens a few times already before the INCOMING_RTL
is assigned.  I thought that might be too pessimistic.

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

I may be plain wrong here, but I thought that !STRICT_ALIGNMENT targets
just use MEM_ALIGN to select the right instructions.  MEM_ALIGN
is always 32-bit align on the DImode memory.  The x86_64 vector instructions
would look at MEM_ALIGN and do the right thing, yes?

It seems to be the definition of STRICT_ALIGNMENT targets that all RTL
instructions need to have MEM_ALIGN >= GET_MODE_ALIGNMENT, so the target
does not even have to look at MEM_ALIGN except in the mov_misalign_optab,
right?

The other hunk, where I admit I did not fully understand the comment, tries
only to increase the MEM_ALIGN to 64-bit if the stack slot is
64-bit aligned although the target said it only needs 32-bit alignment.
So that it is no longer necessary to copy the incoming value.


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

It seems to make us use the incoming register _or_ stack slot if this function
returns true here.

If it returns false here, a new stack slot is allocated, but only if the
original stack slot was not aligned.  This works together with the
other STRICT_ALIGNMENT check in assign_parm_adjust_stack_rtl.
Where also for !STRICT_ALIGNMENT target TYPE_ALIGN and MEM_ALIGN
are checked, but this seems to have only an effect if an address
is taken, in that case I see use_register_for_decl return false
due to TREE_ADDRESSABLE (decl), and whoops, we have an aligned copy
of the unaligned stack slot.

So I believe that there was already a fix for unaligned stack positions,
that relied on the addressability of the parameter, while the target
relied on the 8-byte alignment of the DImode access.

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

Yes, also that the copy is done using movmisalign optab is important.


Thanks
Bernd.