[PATCH] D135687: [AArch64] Fix aligning the stack after calling __chkstk

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1648
+  const bool NeedsRealignment =
+      NumBytes && !IsFunclet && RegInfo->hasStackRealignment(MF);
+  int64_t RealignmentPadding =
----------------
efriedma wrote:
> I'm not sure it actually makes sense to check NumBytes here.  If we're forcing realignment with an attribute, we want to make sure it happens even if there aren't any local variables.
Right, I guess that’s true. However, in the current codepath for non-windows, all alignment is being done within an `if (NumBytes)` condition, so unless there’s further local allocations, it actually won’t realign the stack.

Also, the alignment to apply on the stack (from `-mstack-alignment`) doesn’t seem to be taken into account for actually aligning the stack, only the alignment of the objects in the current stack frame. So this does seem like a preexisting bug.

It seems like the x86 backend does the right thing here (aligning to the maximum of the objects in the stack frame and the stack’s own alignment, while this code here only looks at the objects.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1651
+      (NeedsRealignment && MFI.getMaxAlign() > Align(16))
+          ? MFI.getMaxAlign().value() - 16
+          : 0;
----------------
efriedma wrote:
> I suggested subtracting "16" here on the assumption the stack is actually 16-byte aligned on entry.  If we're not assuming the incoming stack is 16-byte aligned, we can't do that.  (Theoretically, we could subtract 1, but we can't actually allocate in increments of less than 16.)
> 
> Normally, we shouldn't be using realignment in the first place if we're only trying to align the stack to 16 bytes, but I guess the "stackrealign" attribute forces us to realign the stack even if we think it's already aligned.  (By default, the CPU faults if the alignment of sp is less than 16, so that seems unlikely, but I guess there could be some environment which disables that flag.)
I don’t really think we need to worry about the case when the incoming stack isn’t aligned to 16 bytes, I guess thus is just a degenerate case - while it’s only expected to do anything if the requested alignment is higher than that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135687