Fix frame pointer layout on AArch64 Linux.

[resistor]

Summary

When floating point callee-saved registers were used, the frame pointer would incorrectly point to the bottom of the CSR space (containing saved floating-point registers), rather than to the frame record.

While all frame offsets were calculated consistently, resulting in working code, this prevented stack walkers from being about to traverse the frame list.

[echristo]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

1189

Can you comment what's going on here for posterity? :)

1866

Ditto.

[MaskRay]

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

243

Omit braces

http://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

llvm/test/CodeGen/AArch64/framelayout-fp-csr.ll

14

Append a colon to make the label unique test1:

15

FileCheck ignores whitespace change by default. It looks better when CHECK and CHECK-NEXT's content is aligned.

[resistor]

Update for comments.

[echristo]

LGTM from maintainability etc. Haven't really checked more so might want to wait for others.

[resistor]

Landed as a0aed80b22d

[MaskRay]

a0aed80b22d1b698b86e0c16109fdfd4d592756f does not include Differential Revision: so the revision was not closed when the commit landed in HEAD.

If you use arc diff 'HEAD^' to create a patch, arc will set up the tag automatically.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

if (CombineSPBump)
       fixupCalleeSaveRestoreStackOffset(*MBBI, AFI->getLocalStackSize(),
                                         NeedsWinCFI, &HasWinCFI);
     ++MBBI;
   }
 
   // For funclets the FP belongs to the containing function.
   if (!IsFunclet && HasFP) {
     // Only set up FP if we actually need to.
-    int64_t FPOffset = isTargetDarwin(MF) ? (AFI->getCalleeSavedStackSize() - 16) : 0;
+    int64_t FPOffset;
+
+    // The frame pointer needs to point to the location of the frame record
+    // (x28 and x29) within the callee saved register space.
+    if (isTargetDarwin(MF)) {
+      // On Darwin, these are located at the top of the CSR space.
+      FPOffset = (AFI->getCalleeSavedStackSize() - 16);
+    } else {
+      // On other systems, these are located in the middle of the CSR space,
+      // after the other GPRs and before the FPRs.
+      assert(MFI.isCalleeSavedInfoValid() && "CalleeSavedInfo not calculated");
+      if (MFI.getCalleeSavedInfo().empty()) {
+        FPOffset = 0;
+      } else {
+        FPOffset = AFI->getCalleeSavedStackSize(MFI, [](unsigned Reg) {
+          return AArch64::FPR64RegClass.contains(Reg) ||
+                 AArch64::FPR128RegClass.contains(Reg);
+        });
+      }
+    }
 
     if (CombineSPBump)
       FPOffset += AFI->getLocalStackSize();
 
     // Issue    sub fp, sp, FPOffset or
     //          mov fp,sp          when FPOffset is zero.
     // Note: All stores of callee-saved registers are marked as "FrameSetup".
     // This code marks the instruction(s) that set the FP also.

 static StackOffset getFPOffset(const MachineFunction &MF, int64_t ObjectOffset) {
   const auto *AFI = MF.getInfo<AArch64FunctionInfo>();
   const auto &Subtarget = MF.getSubtarget<AArch64Subtarget>();
   bool IsWin64 =
       Subtarget.isCallingConvWin64(MF.getFunction().getCallingConv());
 
   unsigned FixedObject =
       getFixedObjectSize(MF, AFI, IsWin64, /*IsFunclet=*/false);
-  unsigned FPAdjust = isTargetDarwin(MF)
-                        ? 16 : AFI->getCalleeSavedStackSize(MF.getFrameInfo());
+
+  // Compensate for the position of the frame record within the callee-saved
+  // register space.  On Darwin, this is a fixed offset.  On other systems,
+  // this is determined by the number of callee-saved GPRs, excluding FPRs.
+  unsigned FPAdjust =
+      isTargetDarwin(MF)
+          ? 16
+          : AFI->getCalleeSavedStackSize(MF.getFrameInfo(), [](unsigned Reg) {
+              return AArch64::GPR64RegClass.contains(Reg);
+            });
   return {ObjectOffset + FixedObject + FPAdjust, MVT::i8};
 }
 
 static StackOffset getStackOffset(const MachineFunction &MF, int64_t ObjectOffset) {
   const auto &MFI = MF.getFrameInfo();
   return {ObjectOffset + (int64_t)MFI.getStackSize(), MVT::i8};
 }
 

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

public:
   void setCalleeSavedStackSize(unsigned Size) {
     CalleeSavedStackSize = Size;
     HasCalleeSavedStackSize = true;
   }
 
   // When CalleeSavedStackSize has not been set (for example when
   // some MachineIR pass is run in isolation), then recalculate
   // the CalleeSavedStackSize directly from the CalleeSavedInfo.
+  // RegisterFilter is a predicate to calculate the stack size for
+  // subsets of the callee-saved registers.  It should return true
+  // for registers that should be included in the size calculation,
+  // and false otherwise.
   // Note: This information can only be recalculated after PEI
   // has assigned offsets to the callee save objects.
-  unsigned getCalleeSavedStackSize(const MachineFrameInfo &MFI) const {
+  unsigned getCalleeSavedStackSize(
+      const MachineFrameInfo &MFI,
+      llvm::function_ref<bool(unsigned)> RegisterFilter = nullptr) const {
     bool ValidateCalleeSavedStackSize = false;
 
 #ifndef NDEBUG
     // Make sure the calculated size derived from the CalleeSavedInfo
     // equals the cached size that was calculated elsewhere (e.g. in
     // determineCalleeSaves).
     ValidateCalleeSavedStackSize = HasCalleeSavedStackSize;
 #endif
 
-    if (!HasCalleeSavedStackSize || ValidateCalleeSavedStackSize) {
+    if (RegisterFilter || !HasCalleeSavedStackSize ||
+        ValidateCalleeSavedStackSize) {
       assert(MFI.isCalleeSavedInfoValid() && "CalleeSavedInfo not calculated");
       if (MFI.getCalleeSavedInfo().empty())
         return 0;
 
       int64_t MinOffset = std::numeric_limits<int64_t>::max();
       int64_t MaxOffset = std::numeric_limits<int64_t>::min();
+
+      bool AnyRegistersCounted = false;
       for (const auto &Info : MFI.getCalleeSavedInfo()) {
+        if (RegisterFilter) {
+          unsigned Reg = Info.getReg();
+          if (!RegisterFilter(Reg))
+            continue;
+        }
+
+        AnyRegistersCounted = true;
         int FrameIdx = Info.getFrameIdx();
         if (MFI.getStackID(FrameIdx) != TargetStackID::Default)
           continue;
         int64_t Offset = MFI.getObjectOffset(FrameIdx);
         int64_t ObjSize = MFI.getObjectSize(FrameIdx);
         MinOffset = std::min<int64_t>(Offset, MinOffset);
         MaxOffset = std::max<int64_t>(Offset + ObjSize, MaxOffset);
+        AnyRegistersCounted = true;
       }
 
+      if (!AnyRegistersCounted)
+        return 0;
+
       unsigned Size = alignTo(MaxOffset - MinOffset, 16);
-      assert((!HasCalleeSavedStackSize || getCalleeSavedStackSize() == Size) &&
+      assert((RegisterFilter || !HasCalleeSavedStackSize ||
+              getCalleeSavedStackSize() == Size) &&
              "Invalid size calculated for callee saves");
       return Size;
     }
 
     return getCalleeSavedStackSize();
   }
 
   unsigned getCalleeSavedStackSize() const {

llvm/test/CodeGen/AArch64/framelayout-fp-csr.ll

+; RUN: llc -verify-machineinstrs -mtriple=aarch64-none-linux-gnu -disable-post-ra --frame-pointer=all < %s | FileCheck %s
+
+; The purpose of this test is to verify that frame pointer (x29)
+; is correctly setup in the presence of callee-saved floating
+; point registers.  The frame pointer should point to the frame
+; record, which is located 16 bytes above the end of the CSR
+; space when a single FP CSR is in use.
+define void @test1(i32) #26 {
+entry:
+  call void asm sideeffect "nop", "~{d8}"() #26
+  ret void
+}
+; CHECK-LABEL: test1:
+; CHECK:       str     d8, [sp, #-32]!
+; CHECK-NEXT:  stp     x29, x30, [sp, #16]
+; CHECK-NEXT:  add     x29, sp, #16
+; CHECK:       nop
+; CHECK:       ldp     x29, x30, [sp, #16]
+; CHECK-NEXT:  ldr     d8, [sp], #32
+; CHECK-NEXT:  ret
+
+attributes #26 = { nounwind }