[PATCH] D94401: [AArch64][SVE] Allow accesses to SVE stack objects to use frame pointer

[Bradley Smith via Phabricator via llvm-commits]

bsmith marked an inline comment as done.
bsmith added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h:251-253
+  bool isCalleeSavedStackSizeComputed() const {
+    return HasCalleeSavedStackSize;
+  }
----------------
sdesmalen wrote:
> bsmith wrote:
> > sdesmalen wrote:
> > > You should be able to use `MFI.isCalleeSavedInfoValid()` (instead of having to add this function).
> > CalleeSavedInfoValid is set before the callee saved stack size is set, hence unfortunately this isn't sufficient. Doing this triggers an assertion: 
> > 
> >   unsigned int llvm::AArch64FunctionInfo::getCalleeSavedStackSize() const: Assertion `HasCalleeSavedStackSize && "CalleeSavedStackSize has not been calculated"' failed.
> > 
> > Given that the callee saved stack size being set is protected by `HasCalleeSavedStackSize` I think it makes sense to expose that value.
> Are you sure? `MFI.setCalleeSavedInfoValid(true)` is called in PEI after calling `TFI->determineCalleeSaves()` which sets `HasCalleeSavedStackSize`.
You are right about this, I got this backwards. The assertion is actually coming from MIR tests which set `setCalleeSavedInfoValid` to true in `MIRParserImpl::initializeFrameInfo()`. So I guess in normal use this isn't a problem, but for MIR tests it can be.

I'm not sure how to approach this one, I still feel like exposing HasCalleeSavedStackSize is the right thing to do though, as that is protecting the thing we are actually querying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94401