[PATCH] D158190: [wip] AMDGPU: Try to restore SP correctly in presence of dynamic stack adjustments

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D158190#4595893 <https://reviews.llvm.org/D158190#4595893>, @sebastian-ne wrote:

> Just to make sure I understand our current scheme correctly (before this patch).
> If we re-align the stack, we do (ignoring the scale factor)
>
>   fp = sp + (alignment - 1)
>   fp &= -alignment
>   sp += frameSize + alignment
>
> And in the epilogue:
>
>   sp -= frameSize + alignment

Yes, this is essentially what was happening

> Due to the alignment of fp (but not sp), the allocated stack size `sp - fp` may be larger than needed, but it is restored correctly.
> However, sp is not aligned, so maybe this causes problems when calling another function that expects the stack to be already aligned?

We currently assume 16 byte alignment on stack entry. Realignment is triggered by a stack object with a larger alignment requirement, or forced with the "stackrealign" attribute.

I don't think there was any pre-existing issue with stack realignment. We restored the realigned size in the epilog. The problem is if there were any dynamic stack adjustments, the restore using a fixed offset just assumed none happened.

> In case the stack is already aligned and does not need re-alignment, using a `sp = fp` in the epilogue sounds ok to me.
> For the re-alignment case, `sp = fp - (alignment - 1)` looks incorrect to me.
>
> If we do not want the over-commitment, we need to enforce the presence of a base pointer and in the epilogue, restore the stack pointer from the base pointer (`sp = bp`).

The FP is set after the stack is realigned, so the original SP has the additional alignment padding offset


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

https://reviews.llvm.org/D158190