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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
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);
+  };
----------------
fpetrogalli wrote:
> 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... :)
Yes this was deliberate as I think it reads a bit easier than one big expression.


================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp:327
   const AArch64FrameLowering &TFI = *getFrameLowering(MF);
-  return TFI.hasFP(MF);
+  return TFI.hasFP(MF) && !TFI.estimateSVEStackObjectOffsets(MF.getFrameInfo());
 }
----------------
efriedma wrote:
> The problem here, I guess, is the spill slot can't be addressed relative to SP? That makes sense, I guess... but do we also need to check for stack realignment here?
Apologies for my delayed repsonse, I completely forgot about this patch.

> The problem here, I guess, is the spill slot can't be addressed relative to SP?
In that case it will be allocated close to the BP. If there are variable-sized objects the choices are either BP or FP, otherwise the choices are SP or FP. Because the emergency spillslot needs to be accessed using the normal (non-SVE) addressing modes in one go (i.e. without requiring another register to materialize the address to the emergency slot), it needs to be able to access it directly from Pointer + <offset>, so BP or SP are really the only options. Otherwise it would need a temporary register to calculate `FP - sizeof(SVE area)`, which itself would require the emergency slot to materialise the temporary.

> do we also need to check for stack realignment here?
I don't think that's specifically needed, because `hasFP()` will always evaluate to `true` if the stack requires realignment. Do you want me to add an `assert` for that?


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

https://reviews.llvm.org/D70174