Page MenuHomePhabricator

[AArch64] Implement aarch64_vector_pcs codegen support.
ClosedPublic

Authored by sdesmalen on Aug 30 2018, 2:16 AM.

Details

Summary

This patch adds codegen support for the saving/restoring
V8-V23 for functions specified with the aarch64_vector_pcs
calling convention attribute, as added in patch D51477.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.Aug 30 2018, 2:16 AM

Thanks for working on this! Other than the comments I left it looks good to me.

lib/Target/AArch64/AArch64FrameLowering.cpp
539 ↗(On Diff #163283)

Should this be % Scale?

1524 ↗(On Diff #163283)

Would this also work?

for (unsigned Reg : SavedRegs.set_bits())
  Size += TRI.getRegSizeInBits(Reg, MRI);

or something like std::accumulate.

1592 ↗(On Diff #163283)

Why do we need to recalculate this here? Can't we just update it along the way?

test/CodeGen/AArch64/aarch64-vector-pcs.ll
29 ↗(On Diff #163283)

I think one nice way of testing this is using MIR.

You can write things like:

$x19 = IMPLICIT_DEF
$q10 = IMPLICIT_DEF
$q11 = IMPLICIT_DEF

and use llc -run-pass prologepilog or llc -start-before prologepilog to run it.

You can also look at test/CodeGen/AArch64/reverse-csr-restore-seq.mir. You might also want to check the stack: entries for the size and alignment like it's done in test/CodeGen/AArch64/spill-stack-realignment.mir.

sdesmalen marked 2 inline comments as done.Aug 31 2018, 2:53 AM
sdesmalen added inline comments.
lib/Target/AArch64/AArch64FrameLowering.cpp
539 ↗(On Diff #163283)

I think it should, well spotted!

1524 ↗(On Diff #163283)

I tried it and it seems to work, but one of the unit tests fails. I think this is because it exposes a bug where Reg is AArch64::NoRegister (set on line 1515 above to ensure registers are being stored in pairs for MachO's compact unwind format).

1592 ↗(On Diff #163283)

No need to recalculate indeed, I've fixed it now.

test/CodeGen/AArch64/aarch64-vector-pcs.ll
29 ↗(On Diff #163283)

Thanks for the suggestion. When I created the test initially I wasn't convinced that having a .mir test would add much benefit, but I think being able to check the alignment of the objects on the stack is a good reason to do so (rather than observing the effects in the resulting assembly). I've migrated the test to a .mir test in my latest revision.

sdesmalen updated this revision to Diff 163493.Aug 31 2018, 2:56 AM
sdesmalen marked 2 inline comments as done.
  • Some refactoring to maintain CSStackSize.
  • Migrated .ll test to a .mir test.
thegameg added inline comments.
lib/Target/AArch64/AArch64FrameLowering.cpp
1524 ↗(On Diff #163283)

I guess we could check for !Reg in the loop, or fix getRegSizeInBits. I am not sure if returning 0 in getRegSizeInBits would be a good idea but I will let others comment on that.
Either way, it could be a good idea to add a comment + an assert in getRegSizeInBits.

1588 ↗(On Diff #163493)

Could you add a comment explaining that the 8 here is not Scale because we only add GPRs?

sdesmalen added inline comments.Sep 3 2018, 8:10 AM
lib/Target/AArch64/AArch64FrameLowering.cpp
1524 ↗(On Diff #163283)

Rather than fixing it up in this loop, I wonder if the condition 'PairedReg != AArch64::NoRegister' should be added to the condition on line 1513, so that it is never set as a SavedReg to begin with. (an assert is already present in getRegSizeInbits).

In the test that fails: test/CodeGen/AArch64/preserve_mostcc.ll, not all registers are stored in pairs (see str x15), which seems to contradict the reason for setting PairedReg according to the comment on line 1510. I don't really know enough about the MachO format to know whether this is a bug, so perhaps someone else can comment on that. @gberry ?

1588 ↗(On Diff #163493)

yes, will do!

sdesmalen updated this revision to Diff 163767.Sep 4 2018, 2:44 AM
  • Calculate CSStackSize by accumulating reg-size for each saved register.
  • Only sets PairedReg in SavedRegs when it is not AArch64::NoRegister.
  • Removed trailing whitespace from test.
thegameg added inline comments.Sep 4 2018, 2:51 AM
lib/Target/AArch64/AArch64FrameLowering.cpp
1524 ↗(On Diff #163283)

Right, it makes sense not to add it to SavedRegs. For compact unwinding we prefer storing registers in pairs to avoid encoding every single register in the unwind info, but in this case we definitely don't have support for mostcc. The supported registers are: x19 to x28 and d8 to d15, so in this case we will fallback on dwarf. I think this should be enough to fix it:

     // MachO's compact unwind format relies on all registers being stored in
     // pairs.
     // FIXME: the usual format is actually better if unwinding isn't needed.
-    if (produceCompactUnwindFrame(MF) && !SavedRegs.test(PairedReg)) {
+    if (PairedReg != AArch64::NoRegister && produceCompactUnwindFrame(MF) &&
+        !SavedRegs.test(PairedReg)) {
       SavedRegs.set(PairedReg);
       if (AArch64::GPR64RegClass.contains(PairedReg) &&
           !RegInfo->isReservedReg(MF, PairedReg))
thegameg accepted this revision.Sep 4 2018, 2:55 AM

LGTM, thanks!

lib/Target/AArch64/AArch64FrameLowering.cpp
1524 ↗(On Diff #163283)

You were faster than me commenting on this :) Thanks!

This revision is now accepted and ready to land.Sep 4 2018, 2:55 AM
This revision was automatically updated to reflect the committed changes.