This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Spilling/filling of SVE callee-saves.
ClosedPublic

Authored by sdesmalen on Oct 15 2019, 9:59 AM.

Details

Summary

Implement the spills/fills of callee-saved SVE registers using STR and LDR
instructions.

Also adds the aarch64_sve_vector_pcs attribute to specify the
callee-saved registers to be used for functions that return SVE vectors or
take SVE vectors as arguments. The callee-saved registers are vector
registers z8-z23 and predicate registers p4-p15.

The overal frame-layout with SVE will be as follows:

+-------------+
| stack args  |
+-------------+
| Callee Saves|
|   X29, X30  |
|-------------| <- FP
| SVE Callee  | < //////////////
| saved regs  | < //////////////
|    z23      | < //////////////
|     :       | < // SCALABLE //
|    z8       | < //////////////
|    p15      | < /// STACK ////
|     :       | < //////////////
|    p4       | < //// AREA ////
+-------------+ < //////////////
|     :       | < //////////////
|  SVE locals | < //////////////
|     :       | < //////////////
+-------------+
|/////////////| alignment gap.
|     :       |   
| Stack objs  |
|     :       |   
+-------------+ <- SP after call and frame-setup

Diff Detail

Event Timeline

sdesmalen created this revision.Oct 15 2019, 9:59 AM

I'd like to see some more tests for this, in particular:

  • Different combinations of register types being saved, e.g. only P regs without X or Z regs.
  • Check that pairing of other register types still works in the presence of SVE callee-saved registers.
lib/Target/AArch64/AArch64FrameLowering.cpp
640 ↗(On Diff #225059)

Returning without doing anything here looks wrong - code which calls this function will assume that MBBI has been modified to write-back to SP, but this new code path doesn't do that, and has no way to report the failure.

848 ↗(On Diff #225059)

I'm worried that this will start matching load/store instructions which are not part of the prologue/epilogue once we start emitting them. Some calls to this function also check the FrameSetup/FrameDestroy flags, but not all of them. Maybe we could check that the base register is SP, this is run before frame index lowering so only prologue/epilogue code should be referring directly to SP at this point?

lib/Target/AArch64/AArch64MachineFunctionInfo.h
176 ↗(On Diff #225059)

This could do with a comment about what units this is in, I guess it needs to be multiplied by the actual vector length to get the number of bytes?

efriedma added inline comments.Oct 30 2019, 5:10 PM
lib/Target/AArch64/AArch64FrameLowering.cpp
848 ↗(On Diff #225059)

Some code related to call lowering can also directly reference sp. Better to rely on FrameSetup/FrameDestroy.

sdesmalen updated this revision to Diff 227272.Oct 31 2019, 5:45 AM
  • Removed changes to convertCalleeSaveRestoreToSPPrePostIncDec as they were wrong and are no longer necessary.
  • Fixed a possible issue in IsSVECalleeSave, by now checking for FrameSetup/FrameDestroy flags, instead of relying the address is a frame-index.
  • Added a comment to setSVECalleeSavedStackSize.
  • Added test that spills/fills ZPR callee-saved regs in isolation.
  • Added test that spills/fills PPR callee-saved regs in isolation.
  • Extended test save_restore_sve with some GPR callee-saved spills/fills to test that pairing of GPR regs still works in combination with SVE callee saves.
sdesmalen marked 6 inline comments as done.Oct 31 2019, 5:49 AM

I'd like to see some more tests for this, in particular:

  • Different combinations of register types being saved, e.g. only P regs without X or Z regs.
  • Check that pairing of other register types still works in the presence of SVE callee-saved registers.

Thanks for the review! I've added some new tests, please let me know if that satisfies your comment.

lib/Target/AArch64/AArch64MachineFunctionInfo.h
176 ↗(On Diff #225059)

Correct, these are "scalable bytes" which means they will be scaled at runtime by vscale. SVE vectors are vscale * 128bits wide.

ostannard accepted this revision.Nov 4 2019, 5:32 AM

LGTM, thanks

This revision is now accepted and ready to land.Nov 4 2019, 5:32 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2019, 1:07 AM

LGTM, thanks

Thanks for the review @ostannard!