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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1281
 
+  MachineBasicBlock::iterator FirstTerminator = MBB.getFirstTerminator();
+
----------------
greened wrote:
> Maybe this and the uses below are better as a separate NFC patch?
This change is no longer in my updated patch.


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


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1486
+
+  emitFrameOffset(MBB, FirstTerminator, DL, AArch64::SP, AArch64::SP,
+                  {SVEStackSize, MVT::nxv1i8}, TII,
----------------
greened wrote:
> Add a comment here explaining what this is doing.
This change is no longer in my updated patch.


================
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);
----------------
greened wrote:
> It's not clear to me what these are.  Could you name them a bit more clearly, specifically without acronyms?
Good point, I see how this is unclear. I've renamed the variables in my latest revision!


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

https://reviews.llvm.org/D61437