Fix AArch64 AAPCS frame record chain

[logan]

Summary

After the commit r368987 (rG643adb55769e) was landed, the frame record (FP and LR register) may be placed in the middle of a stack frame if a function has both callee-saved general-purpose registers and floating point registers. This will break the stack unwinders that simply walk through the frame records (based on the guarantee from AAPCS64 "The Frame Pointer" section). This commit fixes the problem by adding the frame record offset.

[sdesmalen]

Thanks for creating a fix for this @logan!

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2289

There is a condition here that is not yet tested. If the frame-record is saved that is both LR and FP, not just FP, so is this case needed?

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2289

I think we don't have to check LR here because AAPCS64 guarantees FP and LR will be spilled to consecutive words. Besides, we only care about the address of the spilled FP.

[sdesmalen]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2289

If LR is also spilled, shouldn't isPaired() be true? This is testing two layouts (LR, FP) and (FP, <something else>), where the latter is not a frame-record.

This makes me think that this code also needs a condition for hasFP(MF), because that guarantees the existence of a frame-record (as opposed to ordinary spills of FP/LR that don't necessarily constitute the framerecord)

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2289

I am concerned about the case that FP is the second register in the pair (next one). Since the EmitMI function only gets a RegisterPairInfo. I cannot see the next RegisterPairInfo. Let me think about how to revise this tomorrow.

hasFP(MF) makes sense to me. I'll update the code in the next revision.

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2289

Revised. Please take a look. Thanks.

[sdesmalen]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

1022–1023

Having calculated the offset to the FrameRecord explicitly, can we now replace this with:

int FPOffset = AFI->getFrameRecordOffset();

?

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

Is this description correct? The current meaning of FrameRecordOffset seems to be the offset from SP _after_ allocating the callee-save area.

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

1022–1023

Yes. Replaced in the latest Diff.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

Thanks. Reworded.

[sdesmalen]

Functionally the patch is looking good, nearly there!

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

Thanks! Is it worth renaming the variable to something like OffsetToFrameRecordFromCalleeSaveBase to make the meaning of the variable easier to understand in the places it is used? (I couldn't think of something shorter :))

llvm/test/CodeGen/AArch64/framelayout-frame-record.ll

13

This has some implicit knowledge that d9 and d8 will be used to keep d0 and d1.
This is probably better tested with a MIR test like:

#RUN: llc -mtriple=aarch64-- -start-before prologepilog %s -o - | FileCheck %s
---
name: TestFrameRecordLocation
tracksRegLiveness: true
frameInfo:
  isFrameAddressTaken: true
body: |
  bb.0:
    $d8 = IMPLICIT_DEF
    $d9 = IMPLICIT_DEF
    RET_ReallyLR
...

which explicitly marks d8 and d9 as callee-saved. The isFrameAddressTaken: true will trigger the use of the frame-pointer, and thus storing of the frame-record. You only need to add a few check lines.

14

nit: instead of checking [[FP_OFFSET:16]] as a variable, you can just check 16 directly.

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2072

Just noticed that Windows have different ordering.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

The naming convention in the file seems to be {get,set,}XXXOffset, thus I renamed this to CalleeSaveBaseToFrameRecordOffset.

[sdesmalen]

Thanks @logan , LGTM!

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2072

Yes, good spot!

[logan]

Run git clang-format and resolve rebase conflicts.

I am currently converting my SVN commit access to GitHub access. I'll land this as soon as possible.

[logan]

I reverted the CL because I encountered the following assertion:

assert((DestReg != AArch64::SP || Bytes % 16 == 0) &&
       "SP increment/decrement not 16-byte aligned");

I will upload a revision soon.

[logan]

Rebase to latest LLVM master branch

[sdesmalen]

In D70800#1982722, @logan wrote:

Rebase to latest LLVM master branch

Did you make any other changes other than the rebase?

[logan]

In D70800#1982889, @sdesmalen wrote:

In D70800#1982722, @logan wrote:

Rebase to latest LLVM master branch

Did you make any other changes other than the rebase?

Nope. I am still debugging the assertion failure. One of the emitFrameOffset in emitEpilogue get non-zero NumBytes that is not multiple of 16. But from the limited backtrace, I have some difficulty to spot the problem.

BTW, I cannot locally reproduce it with the same command. On my side, the command can run without problems.

[paulwalker-arm]

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.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

3445–3446 ↗

(On Diff #288386)

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.

[paulwalker-arm]

But doesn't that mean the assert is now protecting less than it was? Without the knowledge of FP's displacement, you cannot know if Bytes % 8 == 0 is safe or not. Is the knowledge of FP's displacement recorded anyway so the original intent of the assert is not compromised? From an SVE point of view, FP displacement will either need to be prevented or undone if it's used to access SVE stack slots. This is because SVE offsets are implicitly scaled and thus any byte based displacement cannot be encoded into its instructions.

It's my believe that at a minimum the assert should be tightened up to pre-D70800 levels before reapplying the patch.

Paul!!!

On 01/09/2020, 16:28, "Owen Anderson" <[e-mail]> wrote:

[paulwalker-arm]

Sure, but that means there need to be a way to detect when FP is displaced (hence my original question). If such data is available (and it needs to be for SVE code generation to function correctly) then I don't see why the assert cannot use it to assert Bytes is correctly aligned and displaced.

On 01/09/2020, 18:29, "Owen Anderson" <[e-mail]> wrote:

The assertion is protecting less than it was, because the original was overly strictly.  Please refer to the test case I mentioned, which provides an example where AAPCS-compliant code generation (frame record layout is part of AAPCS!) is impossible under the original assertion.  As such, I don’t see a path to returning the assertion to its original form.  Again, correct AAPCS-compliant code generation for this test is impossible with the assertion as it was.

I suspect you will find that if you use that test case as a starting point and add an SVE stack object to it, you will find that the SVE frame layout code needs to handle the case of an unaligned frame pointer.  That’s most likely the underlying cause of the failure you were seeing.

[paulwalker-arm]

Fair enough, thanks for the information Owen.

Clearly my preference would be to not fix one AArch64 bug by introducing another. You sound more knowledgeable in this area than myself so perhaps could create your suggested SVE test cases to ensure this isn't the case. That said, if you prefer to just reapply the patch as is, at least we've recorded the issue for others working on SVE.

Paul!!!

On 01/09/2020, 19:12, "Owen Anderson" <[e-mail]> wrote:

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

if (F.hasPersonalityFn()) {
       }
     }
 
     return;
   }
 
   if (HasFP) {
     // Only set up FP if we actually need to.
-    int FPOffset = isTargetDarwin(MF) ? (AFI->getCalleeSavedStackSize() - 16) : 0;
+    int FPOffset = isTargetDarwin(MF) ? (AFI->getCalleeSavedStackSize() - 16)
+                                      : AFI->getFrameRecordOffset();
 
     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.
void AArch64FrameLowering::emitEpilogue(MachineFunction &MF,
   }
 
   // Restore the original stack pointer.
   // FIXME: Rather than doing the math here, we should instead just use
   // non-post-indexed loads for the restores if we aren't actually going to
   // be able to save any instructions.
   if (!IsFunclet && (MFI.hasVarSizedObjects() || AFI->isStackRealigned())) {
     int64_t OffsetToFrameRecord =
-        isTargetDarwin(MF) ? (-(int64_t)AFI->getCalleeSavedStackSize() + 16) : 0;
+        isTargetDarwin(MF) ? (-(int64_t)AFI->getCalleeSavedStackSize() + 16)
+                           : -AFI->getFrameRecordOffset();;
     emitFrameOffset(MBB, LastPopI, DL, AArch64::SP, AArch64::FP,
                     {OffsetToFrameRecord, MVT::i8},
                     TII, MachineInstr::FrameDestroy, false, NeedsWinCFI);
   } else if (NumBytes)
     emitFrameOffset(MBB, LastPopI, DL, AArch64::SP, AArch64::SP,
                     {NumBytes, MVT::i8}, TII, MachineInstr::FrameDestroy, false,
                     NeedsWinCFI);
 
assert(((!RPI.isScalable() && RPI.Offset >= -64 && RPI.Offset <= 63) ||
            "Offset out of bounds for LDP/STP immediate");
 
     RegPairs.push_back(RPI);
     if (RPI.isPaired())
       ++i;
   }
 }
 
 bool AArch64FrameLowering::spillCalleeSavedRegisters(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
     const std::vector<CalleeSavedInfo> &CSI,
     const TargetRegisterInfo *TRI) const {
   MachineFunction &MF = *MBB.getParent();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   bool NeedsWinCFI = needsWinCFI(MF);
   DebugLoc DL;
   SmallVector<RegPairInfo, 8> RegPairs;
auto EmitMI = [&](const RegPairInfo &RPI) {
     // paired.  Make the switch here, so that the code below will save (x,x+1)
     // and not (x+1,x).
     unsigned FrameIdxReg1 = RPI.FrameIdx;
     unsigned FrameIdxReg2 = RPI.FrameIdx + 1;
     if (NeedsWinCFI && RPI.isPaired()) {
       std::swap(Reg1, Reg2);
       std::swap(FrameIdxReg1, FrameIdxReg2);
     }
+    if (RPI.Type == RegPairInfo::GPR) {
+      AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
+      if (Reg1 == AArch64::FP)
+        AFI->setFrameRecordOffset((RPI.Offset + 1) * Size);
+      if (RPI.isPaired() && Reg2 == AArch64::FP)
+        AFI->setFrameRecordOffset(RPI.Offset * Size);
+    }
     MachineInstrBuilder MIB = BuildMI(MBB, MI, DL, TII.get(LdrOpc));
     if (RPI.isPaired()) {
       MIB.addReg(Reg2, getDefRegState(true));
       MIB.addMemOperand(MF.getMachineMemOperand(
           MachinePointerInfo::getFixedStack(MF, FrameIdxReg2),
           MachineMemOperand::MOLoad, Size, Align));
     }
     MIB.addReg(Reg1, getDefRegState(true))

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

class AArch64FunctionInfo final : public MachineFunctionInfo {
   /// that must be forwarded to every musttail call.
   SmallVector<ForwardedRegister, 1> ForwardedMustTailRegParms;
 
   // Offset from SP-at-entry to the tagged base pointer.
   // Tagged base pointer is set up to point to the first (lowest address) tagged
   // stack slot.
   unsigned TaggedBasePointerOffset = 0;
 
+  // Offset from SP-at-entry to frame record (i.e. the spilled frame pointer
+  // and link address).
+  int FrameRecordOffset = 0;
+
 public:
   AArch64FunctionInfo() = default;
 
   explicit AArch64FunctionInfo(MachineFunction &MF) {
     (void)MF;
 
     // If we already know that the function doesn't have a redzone, set
     // HasRedZone here.
#endif
 
   unsigned getTaggedBasePointerOffset() const {
     return TaggedBasePointerOffset;
   }
   void setTaggedBasePointerOffset(unsigned Offset) {
     TaggedBasePointerOffset = Offset;
   }
 
+  int getFrameRecordOffset() const {
+    return FrameRecordOffset;
+  }
+  void setFrameRecordOffset(int Offset) {
+    FrameRecordOffset = Offset;
+  }
+
 private:
   // Hold the lists of LOHs.
   MILOHContainer LOHContainerSet;
   SetOfInstructions LOHRelated;
 
   DenseMap<int, std::pair<unsigned, MCSymbol *>> JumpTableEntryInfo;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_AARCH64_AARCH64MACHINEFUNCTIONINFO_H

llvm/test/CodeGen/AArch64/framelayout-frame-record.ll

+; RUN: llc  < %s -mtriple=aarch64--linux-gnu | FileCheck %s
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux-gnu"
+
+declare void @escape.i64(i64)
+declare void @escape.double(double)
+
+define void @test_frame_record_offset(i64 %a, i64 %b, double %c, double %d) "no-frame-pointer-elim"="true" {
+; CHECK-LABEL: test_frame_record_offset:
+; CHECK: stp x29, x30, [sp, #[[OFFSET:[0-9]+]]]
+; CHECK: add x29, sp, #[[OFFSET]]
+
+  call void @escape.i64(i64 %a)
+  call void @escape.i64(i64 %b)
+  call void @escape.double(double %c)
+  call void @escape.double(double %d)
+  ret void
+}