[PATCH] D68996: [AArch64][SVE] Spilling/filling of SVE callee-saves.

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added a comment.

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.



================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:640
     llvm_unreachable("Unexpected callee-save save/restore opcode!");
+  case AArch64::STR_ZXI:
+  case AArch64::STR_PXI:
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:848
+  case AArch64::LDR_PXI:
+    return !I->getOperand(1).isFI();
+  }
----------------
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?


================
Comment at: lib/Target/AArch64/AArch64MachineFunctionInfo.h:176
 
+  void setSVECalleeSavedStackSize(unsigned Size) {
+    SVECalleeSavedStackSize = Size;
----------------
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68996/new/

https://reviews.llvm.org/D68996