[PATCH] D96004: [AArch64] Stack probing for function prologues

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

Thanks for this patch Oliver!
I've started reviewing, but am not very far yet.
I thought I'd share my thoughts so far already, as I'm not sure exactly when I'll be able to do a complete review.



================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1351
 
-  StackOffset AllocateBefore = SVEStackSize, AllocateAfter = {};
+  StackOffset AllocateBefore = {}, AllocateAfter = SVEStackSize;
   MachineBasicBlock::iterator CalleeSavesBegin = MBBI, CalleeSavesEnd = MBBI;
----------------
I know this is not your original code, but I'd be tempted to go for more descriptive names than AllocateBefore and AllocateAfter. It's been a little while since I looked at the FrameLowering code, and from the names alone, I could not guess what these stack offsets were meant to represent.
Maybe I'll have a suggestion for a better name later on after I've read more of the patch.

I think it also makes it easier to read this code if there was a comment above this that describes that this part of the code handles SVE CSR and SVE locals (i.e. the SVE area of the stack frame).


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1351
 
-  StackOffset AllocateBefore = SVEStackSize, AllocateAfter = {};
+  StackOffset AllocateBefore = {}, AllocateAfter = SVEStackSize;
   MachineBasicBlock::iterator CalleeSavesBegin = MBBI, CalleeSavesEnd = MBBI;
----------------
kristof.beyls wrote:
> I know this is not your original code, but I'd be tempted to go for more descriptive names than AllocateBefore and AllocateAfter. It's been a little while since I looked at the FrameLowering code, and from the names alone, I could not guess what these stack offsets were meant to represent.
> Maybe I'll have a suggestion for a better name later on after I've read more of the patch.
> 
> I think it also makes it easier to read this code if there was a comment above this that describes that this part of the code handles SVE CSR and SVE locals (i.e. the SVE area of the stack frame).
I'm a bit confused by this code change (probably partly due to the not really understanding what these variables are meant to represent).
Does the change on this line indicate that there was a bug here before? If so, would it be better to commit that separately?
If not, does the code change result in the meaning of AllocateBefore and AllocateAfter to change?


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1382
+  if (TLI.hasInlineStackProbe(MF) && AllocateAfter) {
+    Register ScratchReg = findScratchNonCalleeSaveRegister(&MBB);
+    assert(ScratchReg != AArch64::NoRegister);
----------------
This line of code reminds me that frame setup can also happen in non-entry basic blocks, when shrink wrapping has happened.
Assuming I checked correctly, there are no test cases in the tests in this patch verifying correct behavior in case shrink wrapping happens. I'm not sure if it's worthwhile to have such tests, but I thought I'd check if you thought about this.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1398-1400
+    bool NeedsStackProbe = TLI.hasInlineStackProbe(MF) &&
+                           (NumBytes >= TLI.getStackProbeMaxUnprobedStack(MF) ||
+                            MFI.hasVarSizedObjects());
----------------
This seems to be a key heuristic/logic on when to generate stack probes and when not.
I think it would be useful to have some light design documentation/rationale on why this is the right heuristic.
Depending on the length of that documentation, it could go either here, in the large comment at the top of this file or somewhere else (potentially as a target-independent design doc in the docs directory)? I don't think the design doc has to be particularly long, but a small doc probably will bring a lot of value for future developers needing to touch this code.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.h:153
+  /// Replace a StackProbe stub (if any) with the actual probe code inline
+  virtual void inlineStackProbe(MachineFunction &MF,
+                                MachineBasicBlock &PrologueMBB) const override;
----------------
ostannard wrote:
> serge-sans-paille wrote:
> > Why did you set that one virtual?
> It's an override of a virtual function called by the target-independent part of the prologue/epilogue pass.
I think the keyword virtual is not needed when override is specified.
There are other examples of override methods in this class; they do not explicitly write virtual. So I guess it's more consistent to not use virtual here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96004