This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Correctly align HFA arguments when passed on the stack
ClosedPublic

Authored by chill on Mar 17 2021, 10:25 AM.

Details

Summary

When we pass a Homogeneous Floating-Point Aggregate (HFA) argument
with increased alignment requirements, for example

struct S {
  __attribute__ ((__aligned__(16))) double v[4];
};

Clang uses [4 x double] for the parameter, which is passed on the
stack at alignment 8, whereas it should be at alignment 16, following
Rule C.4 in AAPCS (https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#642parameter-passing-rules)

Currently we don't have a way to express in LLVM IR the alignment
requirements of the function arguments. The align attribute is
applicable to pointers only, and only for some special ways of passing
arguments (e..g byval). When implementing AAPCS32/AAPCS64, clang
resorts to dubious hacks of coercing to types, which naturally have
the needed alignment. We don't have enough types to cover all the
cases, though.

This patch introduces a new use of the stackalign attribute to
control stack slot alignment, when and if an argument is passed in
memory.

The attribute align is left as an optimizer hint - it still applies
to pointer types only and pertains to the content of the pointer,
whereas the alignment of the pointer itself is determined by the
stackalign attribute.

For byval arguments, the stackalign attribute assumes the role,
previously perfomed by align, falling back to align if
stackalign` is absent.

Patch by Momchil Velikov and Lucas Prates.

Diff Detail

Event Timeline

chill created this revision.Mar 17 2021, 10:25 AM
chill requested review of this revision.Mar 17 2021, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2021, 10:25 AM
chill added inline comments.Mar 22 2021, 4:41 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
9428

Maybe we shouldn't be using OrigAlign and can reuse ByValOrByRefAlign (renamed).

chill planned changes to this revision.Mar 22 2021, 7:33 AM
chill updated this revision to Diff 332386.Mar 22 2021, 11:54 AM
t.p.northover accepted this revision.Mar 23 2021, 2:43 AM

I think this looks reasonable. Preserving Darwin is probably the safest choice, though I don't know if we actually rely on it. If the description is actually the commit message make sure it's updated before committing because this patch does in fact change the Clang side too.

This revision is now accepted and ready to land.Mar 23 2021, 2:43 AM

After doing the prototype implementation here https://reviews.llvm.org/D100397
I'm now reasonably convinced it minimally affects *this* patch, and so will go ahead and commit it.

chill requested review of this revision.Apr 13 2021, 10:48 AM
This revision is now accepted and ready to land.Apr 13 2021, 10:48 AM
chill updated this revision to Diff 337475.Apr 14 2021, 8:50 AM
chill edited the summary of this revision. (Show Details)

Updated with only whitespace changes.

This revision was landed with ongoing or failed builds.Apr 15 2021, 2:58 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 2:58 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript