[PATCH] D45770: [AArch64] Disable spill slot scavenging when stack realignment required.

[Geoff Berry via Phabricator via llvm-commits]

gberry added a comment.

In https://reviews.llvm.org/D45770#1072174, @paulwalker-arm wrote:

> In https://reviews.llvm.org/D45770#1071047, @gberry wrote:
>
> > Ugh, good find.  Isn't this an issue if variable sized stack objects are present as well?
>
>
> The graphic (within the same file) showing the stack layout suggests a base pointer is used in such circumstances, so my guess is no.


Yeah, I took a closer look and it appears that either the BP or FP will be used in this case, which should be okay.

I also took a look at fixing this failure by using the FP as a base for cases like this, and it turned out to be not too bad:

  --- a/lib/Target/AArch64/AArch64FrameLowering.cpp
  +++ b/lib/Target/AArch64/AArch64FrameLowering.cpp
  @@ -1019,6 +1019,13 @@ int AArch64FrameLowering::resolveFrameIndexReference(const MachineFunction &MF,
     int FPOffset = MFI.getObjectOffset(FI) + FixedObject + 16;
     int Offset = MFI.getObjectOffset(FI) + MFI.getStackSize();
     bool isFixed = MFI.isFixedObjectIndex(FI);
  +  bool isCSR = !isFixed && MFI.getObjectOffset(FI) >=
  +                               -((int)AFI->getCalleeSavedStackSize());
  
     // Use frame pointer to reference fixed objects. Use it for locals if
     // there are VLAs or a dynamically realigned SP (and thus the SP isn't
  @@ -1032,6 +1034,12 @@ int AArch64FrameLowering::resolveFrameIndexReference(const MachineFunction &MF,
       // Argument access should always use the FP.
       if (isFixed) {
         UseFP = hasFP(MF);
  +    } else if (isCSR && RegInfo->needsStackRealignment(MF)) {
  +      // References to the CSR area must use FP if we're re-aligning the stack
  +      // since the dynamically-sized alignment padding is between the SP/BP and
  +      // the CSR area.
  +      assert(hasFP(MF) && "Re-aligned stack must have frame pointer");
  +      UseFP = true;
       } else if (hasFP(MF) && !RegInfo->needsStackRealignment(MF)) {
         // If the FPOffset is negative, we have to keep in mind that the
         // available offset range for negative offsets is smaller than for
  @@ -1065,9 +1078,9 @@ int AArch64FrameLowering::resolveFrameIndexReference(const MachineFunction &MF,
       }
     }
  
  -  assert((isFixed || !RegInfo->needsStackRealignment(MF) || !UseFP) &&
  +  assert(((isFixed || isCSR) || !RegInfo->needsStackRealignment(MF) || !UseFP) &&
            "In the presence of dynamic stack pointer realignment, "
  -         "non-argument objects cannot be accessed through the frame pointer");
  +         "non-argument/CSR objects cannot be accessed through the frame pointer");
  
     if (UseFP) {
       FrameReg = RegInfo->getFrameRegister(MF);

@t.p.northover should probably approve whichever approach we take since presumably he will need to approve it for 6.0.1 anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D45770