[PATCH] D61437: [AArch64] Static (de)allocation of SVE stack objects.

[David Greene via Phabricator via llvm-commits]

greened added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:187
 
+static uint64_t getSVEStackSize(const MachineFunction &MF) {
+  const AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
----------------
Needs a comment explaining what this does.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:865
     assert(!HasFP && "unexpected function without stack frame but with FP");
+    assert(!SVEStackSize && "Must have stack frame with SVE");
     // All of the stack allocation is for locals.
----------------
This is confusing.  Asserting that `SVEStackSize` is non-zero but the message sort of implies it must be true.  Maybe word this similarly to the assert right above: "unexpected function without stack frame but with SVE objects."


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1281
 
+  MachineBasicBlock::iterator FirstTerminator = MBB.getFirstTerminator();
+
----------------
Maybe this and the uses below are better as a separate NFC patch?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1436
                     MachineInstr::FrameDestroy, false, NeedsWinCFI);
-    if (Done) {
+    if (!SVEStackSize && Done) {
       if (NeedsWinCFI)
----------------
It would be helpful to have a comment here explaining why we are not `Done` if `SVEStackSize` is non-zero.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1486
+
+  emitFrameOffset(MBB, FirstTerminator, DL, AArch64::SP, AArch64::SP,
+                  {SVEStackSize, MVT::nxv1i8}, TII,
----------------
Add a comment here explaining what this is doing.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3052
                            bool NeedsWinCFI) {
-  int64_t Bytes;
-  Offset.getForFrameOffset(Bytes);
+  int64_t Bytes, PLSized, VLSized;
+  Offset.getForFrameOffset(Bytes, PLSized, VLSized);
----------------
It's not clear to me what these are.  Could you name them a bit more clearly, specifically without acronyms?


================
Comment at: lib/Target/AArch64/AArch64StackOffset.h:97
   /// For non-scalable offsets this is simply its byte size.
-  void getForFrameOffset(int64_t &ByteSized) const {
+  void getForFrameOffset(int64_t &ByteSized, int64_t &PLSized,
+                         int64_t &VLSized) const {
----------------
Again, `PL` and `VL` are not very clear.


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

https://reviews.llvm.org/D61437