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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1648
+  const bool NeedsRealignment =
+      NumBytes && !IsFunclet && RegInfo->hasStackRealignment(MF);
+  int64_t RealignmentPadding =
----------------
mstorsjo wrote:
> 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.
Not sure I understand the different sources of alignment, but we can leave that for a followup in any case.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1651
+      (NeedsRealignment && MFI.getMaxAlign() > Align(16))
+          ? MFI.getMaxAlign().value() - 16
+          : 0;
----------------
mstorsjo wrote:
> 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.
In that case, should we just pretend the user didn't request stack realignment if they don't request alignment higher than 16?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135687