[PATCH] D70800: Fix AArch64 AAPCS frame record chain

[Owen Anderson via llvm-commits]

> On Sep 1, 2020, at 8:13 AM, Paul Walker via Phabricator via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> paulwalker-arm added a comment.
> 
> I've had to revert this patch because it caused runtime failures when building spec2k/eon with `-march=armv8-a+sve -mllvm -aarch64-sve-vector-bits-min=256`.
> 
> 
> 
> ================
> Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:3445-3446
>  if (Bytes || (!Offset && SrcReg != DestReg)) {
> -    assert((DestReg != AArch64::SP || Bytes % 16 == 0) &&
> -           "SP increment/decrement not 16-byte aligned");
> +    assert((DestReg != AArch64::SP || Bytes % 8 == 0) &&
> +           "SP increment/decrement not 8-byte aligned");
>    unsigned Opc = SetNZCV ? AArch64::ADDSXri : AArch64::ADDXri;
> ----------------
> I don't believe this is a safe fix for the issue mentioned when the patch was previously reverted by https://reviews.llvm.org/rG04879086b44348cad600a0a1ccbe1f7776cc3cf9.
> 
> The stack always being 16-byte align is a requirement for the AAPCS.
> 

Your claim here is incorrect, as discussed in the discussion Martin and I had before landing it.

The assertion is about how the stack pointer is computed, not how it is aligned.  The patch provides an example of a test case where it necessary in test/CodeGen/AArch64/framelayout-unaligned-fp.ll.  The *frame pointer* is unaligned in this testcase, which means that the stack pointer is then setup from it by subtracting a non-multiple of 16.

Unless you are prepared to debug the SVE failure soon, I believe this patch should be reapplied.  It is correct with respect to AAPCS, and as far as I can tell SVE support is insufficiently tested in public to require that all patches must not break arbitrary variants of it.

—Owen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200901/60aa5dc5/attachment.html>