[PATCH] D70174: [AArch64][SVE] Use FP for scavenging slot

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2522
 // Min/MaxCSFrameIndex, respectively.
 // Returns the size of the stack.
+static int64_t
----------------
I think you should update the description of the method to reflect the new interface. 


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2584-2585
   int MinCSFrameIndex, MaxCSFrameIndex;
-  return determineSVEStackObjectOffsets(MFI, MinCSFrameIndex, MaxCSFrameIndex, false);
+  auto Assign = [](int FI, int64_t Offset) {};
+  auto Align = [](int FI, unsigned Align) {};
+  return determineSVEStackObjectOffsets(MFI, MinCSFrameIndex, MaxCSFrameIndex,
----------------
I think you can remove these two and pass them directly inside the call as `[](int, int64_t){}` and `[](int, unsigned){}`

You could actually make these two values the default argument of the method, so you don't have to bother specifying them here? (up to you, though)


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2592-2598
+  auto Assign = [&MFI](int FI, int64_t Offset) {
+    LLVM_DEBUG(dbgs() << "alloc FI(" << FI << ") at SP[" << Offset << "]\n");
+    MFI.setObjectOffset(FI, Offset);
+  };
+  auto Align = [&MFI](int FI, unsigned Align) {
+    MFI.setObjectAlignment(FI, Align);
+  };
----------------
Nit: these should also be passed directly as argument to the call, no need to set up variables that are not used anywhere else.

I say this is a nit because the first lambda is 4 lines long, so it might not look beautiful once placed inside the function parameters... :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70174