[AArch64][DebugInfo] Do not recompute CalleeSavedStackSize

[sdesmalen]

Summary

This patch fixes a bug exposed by D65653 where a subsequent invocation
of determineCalleeSaves ends up with a different size for the callee
save area, leading to different frame-offsets in debug information.

In the invocation by PEI, determineCalleeSaves tries to determine
whether it needs to spill an extra callee-saved register to get an
emergency spill slot. To do this, it calls 'estimateStackSize' and
manually adds the size of the callee-saves to this. PEI then allocates
the spill objects for the callee saves and the remaining frame layout
is calculated accordingly.

A second invocation in LiveDebugValues causes estimateStackSize to return
the size of the stack frame including the callee-saves. Given that the
size of the callee-saves is added to this, these callee-saves are counted
twice, which leads determineCalleeSaves to believe the stack has
become big enough to require spilling an extra callee-save as emergency
spillslot. It then updates CalleeSavedStackSize with a larger value.

Since CalleeSavedStackSize is used in the calculation of the frame
offset in getFrameIndexReference, this leads to incorrect offsets for
variables/locals when this information is recalculated after PEI.

[efriedma]

Why are we calling determineCalleeSaves in LiveDebugValues, anyway? Can't it just call getCalleeSavedInfo()?

Making the determineCalleeSaves computation lazy, like this patch does, is really confusing.

[sdesmalen]

In D66935#1651499, @efriedma wrote:

Why are we calling determineCalleeSaves in LiveDebugValues, anyway? Can't it just call getCalleeSavedInfo()?
Making the determineCalleeSaves computation lazy, like this patch does, is really confusing.

This method seems to be called by several passes:

• LiveDebugValues
• PrologEpilogInserter
• RegUsageInfoCollector
• RegisterScavenging
• ShrinkWrap

The problem with determineCalleeSaves is that it does much more than determining the callee-saves:

• It also creates a new StackObject for the emergency spill slot.
• It marks this slot as the scavenging slot in the RegisterScavenger.
• It saves the size of the callee-save area (in AArch64MachineFunctionInfo).

And this behaviour to change state seems somewhat by design, because RegisterScavenger* is passed as argument and MachineFunction is not a const reference. After PEI, getCalleeSavedInfo can be used as you suggest, but one could argue that determineCalleeSaves is still a valid interface to use. Perhaps I could add an assert in determineCalleeSaves that suggests using getCalleeSavedInfo if MFI.isCalleeSavedInfoValid() is true, but I think that is even more confusing. Perhaps the better way is to not let determineCalleeSaves change state at all, but that would require changes to the interface, which is likely to also require changes to other targets. From that point of view, letting determineCalleeSaves use the cached information from getCalleeSavedInfo seemed sensible.

[efriedma]

This method seems to be called by several passes:

PrologEpilogInserter is expected to do this; it needs both the information, and the side-effects which modify the stack layout. And the call in RegisterScavenging is actually a unit-test which is trying to fake the state computed by PEI. So really, there are three places which are using the API after PrologEpilogInserter.

None of the places that use the API after PEI want any side-effects. And none of them actually use the functionality to update the RegisterScavenger state. LiveDebugValues and RegUsageInfoCollector both appear to just be using determineCalleeSaves as a bad substitute for getCalleeSavedInfo.

one could argue that determineCalleeSaves is still a valid interface to use

It seems much simpler to just say it isn't a valid interface to use after PEI.

[sdesmalen]

• Introduced a new method getCalleeSaves that fills in a BitVector with the callee-saves if isCalleeSavedInfoValid() is true.
• Replaced uses of determineCalleeSaves with getCalleeSaves in LiveDebugValues, RegUsageInfoCollector and RegisterScavenging.

[efriedma]

This seems much better.

Did you intentionally skip updating ShrinkWrap? Should be straightforward. (There's a bunch of places in that file that reference a RegScavenger, but nothing actually uses it.)

llvm/include/llvm/CodeGen/TargetFrameLowering.h

294

Maybe add a comment here that this shouldn't be called by anything outside of PEI.

llvm/lib/CodeGen/RegisterScavenging.cpp

802

ScavengerTest might actually want the real determineCalleeSaves? In this context, it's basically acting as a substitute for PEI.

[sdesmalen]

In D66935#1656470, @efriedma wrote:

This seems much better.

Thanks!

Did you intentionally skip updating ShrinkWrap? Should be straightforward. (There's a bunch of places in that file that reference a RegScavenger, but nothing actually uses it.)

Yes, ShrinkWrap still needs to use the determineCalleeSaves interface because it is run before PEI when CalleeSavedInfo in MachineFrameInfo has not yet been populated (and thus MFI.isCalleeSavedInfoValid() will return false).

[efriedma]

LGTM

I'm afraid there's a latent bug in the interaction between ShrinkWrap and PEI, but I guess the effect might be sort of hard to spot; if we allocate an unnecessary stack object before PEI, it would be hard to notice.

[sdesmalen]

Just before committing this patch, I realised that I had not yet tested it for all targets (I did this on a different machine than I normally use, hence the change in config).

This led to a few more unit-test failures and thus some small changes to the patch to fix those:

• Made getCalleeSaves return an empty BitVector instead of asserting that CalleeSavedInfo.isValid(). The assert broke several tests that ran the LiveDebugValues pass in isolation. The MIR Parser only calls setCalleeSavedInfoValid(true) when there is at least one callee-save. So without any callee-saves specified in the MIR file, the assert would otherwise still trigger. It may be worth fixing this in a separate patch by e.g. adding an extra field to the MIR.

Note that most of the tests did not require the callee-saved info for the thing they were testing. For two of them,test/DebugInfo/MIR/{Mips,X86}/live-debug-values-reg-copy.mir, it was fixed by letting it start-before=prologepilog to populate the callee-saved info.

• Made getCalleeSaves a virtual function. D64986 introduced a change for the ARM target to mark R0 as callee saved in determineCalleeSaves if the parameter passed in R0 is returned as-is (determined from the returned parameter attribute). Given that R0 is not an actual callee-saved register, the register does not end up in the CalleeSaved list in MachineFrameInfo. By making getCalleeSaves a virtual function, the ARM target can still mark R0 as a preserved register.

@efriedma, are you happy with these changes?

[omjavaid]

Hi

This change was reverted and hence causes LLDB AArch64 test failures again.

Revert action documented here : https://reviews.llvm.org/D67710

[efriedma]

Thanks for pointing that out.

[sdesmalen]

Yes, thanks for pointing out! I've created a fix for the issue in D68783.

llvm/include/llvm/CodeGen/TargetFrameLowering.h

public:
   virtual int getNonLocalFrameIndexReference(const MachineFunction &MF,
                                        int FI) const {
     // By default, dispatch to getFrameIndexReference. Interested targets can
     // override this.
     unsigned FrameReg;
     return getFrameIndexReference(MF, FI, FrameReg);
   }
 
+  /// Returns the callee-saved registers as computed by determineCalleeSaves
+  /// in the BitVector \p SavedRegs.
+  void getCalleeSaves(const MachineFunction &MF, BitVector &SavedRegs) const;
+
   /// This method determines which of the registers reported by
   /// TargetRegisterInfo::getCalleeSavedRegs() should actually get saved.
   /// The default implementation checks populates the \p SavedRegs bitset with
   /// all registers which are modified in the function, targets may override
   /// this function to save additional registers.
   /// This method also sets up the register scavenger ensuring there is a free
   /// register or a frameindex available.
   virtual void determineCalleeSaves(MachineFunction &MF, BitVector &SavedRegs,
                                     RegScavenger *RS = nullptr) const;
 
   /// processFunctionBeforeFrameFinalized - This method is called immediately
   /// before the specified function's frame layout (MF.getFrameInfo()) is
   /// finalized.  Once the frame is finalized, MO_FrameIndex operands are
   /// replaced with direct constants.  This method is optional.
   ///

llvm/lib/CodeGen/LiveDebugValues.cpp

bool LiveDebugValues::runOnMachineFunction(MachineFunction &MF) {
   // Skip functions from NoDebug compilation units.
   if (MF.getFunction().getSubprogram()->getUnit()->getEmissionKind() ==
       DICompileUnit::NoDebug)
     return false;
 
   TRI = MF.getSubtarget().getRegisterInfo();
   TII = MF.getSubtarget().getInstrInfo();
   TFI = MF.getSubtarget().getFrameLowering();
-  TFI->determineCalleeSaves(MF, CalleeSavedRegs,
-                            std::make_unique<RegScavenger>().get());
+  TFI->getCalleeSaves(MF, CalleeSavedRegs);
   LS.initialize(MF);
 
   bool Changed = ExtendRanges(MF);
   return Changed;
 }

llvm/lib/CodeGen/RegUsageInfoCollector.cpp

public:
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<PhysicalRegisterUsageInfo>();
     AU.setPreservesAll();
     MachineFunctionPass::getAnalysisUsage(AU);
   }
 
   bool runOnMachineFunction(MachineFunction &MF) override;
 
-  // Call determineCalleeSaves and then also set the bits for subregs and
+  // Call getCalleeSaves and then also set the bits for subregs and
   // fully saved superregs.
   static void computeCalleeSavedRegs(BitVector &SavedRegs, MachineFunction &MF);
 
   static char ID;
 };
 
 } // end of anonymous namespace
 

 
 void RegUsageInfoCollector::
 computeCalleeSavedRegs(BitVector &SavedRegs, MachineFunction &MF) {
   const TargetFrameLowering &TFI = *MF.getSubtarget().getFrameLowering();
   const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
 
   // Target will return the set of registers that it saves/restores as needed.
   SavedRegs.clear();
-  TFI.determineCalleeSaves(MF, SavedRegs);
+  TFI.getCalleeSaves(MF, SavedRegs);
   if (SavedRegs.none())
     return;
 
   // Insert subregs.
   const MCPhysReg *CSRegs = TRI.getCalleeSavedRegs(&MF);
   for (unsigned i = 0; CSRegs[i]; ++i) {
     MCPhysReg Reg = CSRegs[i];
     if (SavedRegs.test(Reg)) {
       // Save subregisters
       for (MCSubRegIterator SR(Reg, &TRI); SR.isValid(); ++SR)
         SavedRegs.set(*SR);
     }
   }
 }

llvm/lib/CodeGen/RegisterScavenging.cpp

bool runOnMachineFunction(MachineFunction &MF) override {
     const TargetSubtargetInfo &STI = MF.getSubtarget();
     const TargetFrameLowering &TFL = *STI.getFrameLowering();
 
     RegScavenger RS;
     // Let's hope that calling those outside of PrologEpilogueInserter works
     // well enough to initialize the scavenger with some emergency spillslots
     // for the target.
     BitVector SavedRegs;
-    TFL.determineCalleeSaves(MF, SavedRegs, &RS);
+    TFL.getCalleeSaves(MF, SavedRegs);
     TFL.processFunctionBeforeFrameFinalized(MF, &RS);
 
     // Let's scavenge the current function
     scavengeFrameVirtualRegs(MF, RS);
     return true;
   }
 };
 
 } // end anonymous namespace
 
 char ScavengerTest::ID;
 
 INITIALIZE_PASS(ScavengerTest, "scavenger-test",
                 "Scavenge virtual registers inside basic blocks", false, false)

llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp

return MFI.getObjectOffset(FI) + MFI.getStackSize() -
          getOffsetOfLocalArea() + MFI.getOffsetAdjustment();
 }
 
 bool TargetFrameLowering::needsFrameIndexResolution(
     const MachineFunction &MF) const {
   return MF.getFrameInfo().hasStackObjects();
 }
 
+void TargetFrameLowering::getCalleeSaves(const MachineFunction &MF,
+                                         BitVector &CalleeSaves) const {
+  const MachineFrameInfo &MFI = MF.getFrameInfo();
+  assert(MFI.isCalleeSavedInfoValid() &&
+         "Callee saved registers not yet determined");
+
+  const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
+  CalleeSaves.resize(TRI.getNumRegs());
+  for (const CalleeSavedInfo &Info : MFI.getCalleeSavedInfo())
+    CalleeSaves.set(Info.getReg());
+}
+
 void TargetFrameLowering::determineCalleeSaves(MachineFunction &MF,
                                                BitVector &SavedRegs,
                                                RegScavenger *RS) const {
   const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
 
   // Resize before the early returns. Some backends expect that
   // SavedRegs.size() == TRI.getNumRegs() after this call even if there are no
   // saved registers.

 
 int TargetFrameLowering::getInitialCFAOffset(const MachineFunction &MF) const {
   llvm_unreachable("getInitialCFAOffset() not implemented!");
 }
 
 unsigned TargetFrameLowering::getInitialCFARegister(const MachineFunction &MF)
     const {
   llvm_unreachable("getInitialCFARegister() not implemented!");
-}
+}
 

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

void AArch64FrameLowering::determineCalleeSaves(MachineFunction &MF,
 
   // Adding the size of additional 64bit GPR saves.
   CSStackSize += 8 * (SavedRegs.count() - NumSavedRegs);
   unsigned AlignedCSStackSize = alignTo(CSStackSize, 16);
   LLVM_DEBUG(dbgs() << "Estimated stack frame size: "
                << EstimatedStackSize + AlignedCSStackSize
                << " bytes.\n");
 
+  assert((!MFI.isCalleeSavedInfoValid() ||
+          AFI->getCalleeSavedStackSize() == AlignedCSStackSize) &&
+         "Should not invalidate callee saved info");
+
   // Round up to register pair alignment to avoid additional SP adjustment
   // instructions.
   AFI->setCalleeSavedStackSize(AlignedCSStackSize);
   AFI->setCalleeSaveStackHasFreeSpace(AlignedCSStackSize != CSStackSize);
 }
 
 bool AArch64FrameLowering::enableStackSlotScavenging(
     const MachineFunction &MF) const {

llvm/test/CodeGen/AArch64/wrong-callee-save-size-after-livedebugvariables.mir

+# RUN: llc -start-before=prologepilog -filetype=obj -o %t %s
+# RUN: llvm-dwarfdump --name=obj1 %t | FileCheck %s --check-prefix=CHECKDWARF1
+# RUN: llvm-dwarfdump --name=obj2 %t | FileCheck %s --check-prefix=CHECKDWARF2
+# RUN: llvm-objdump --disassemble %t | FileCheck %s --check-prefix=CHECKASM
+#
+# Test that the location for obj1 and obj2 in the debug information is
+# the same as the location used by load instructions.
+#
+# CHECKDWARF1: DW_AT_location    (DW_OP_fbreg -1)
+# CHECKDWARF2: DW_AT_location    (DW_OP_fbreg -2)
+# CHECKASM: ldurb   w0, [x29, #-1]
+# CHECKASM: ldurb   w1, [x29, #-2]
+--- |
+  ; ModuleID = 'wrong-callee-save-size-after-livedebugvariables.c'
+  source_filename = "wrong-callee-save-size-after-livedebugvariables.c"
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64-unknown-linux-gnu"
+
+  ; Function Attrs: noinline nounwind optnone
+  define dso_local i8 @foo() #0 !dbg !7 {
+  entry:
+    %obj1 = alloca i8, align 1
+    %obj2 = alloca i8, align 1
+    %obj3 = alloca [238 x i8], align 1
+    ret i8 undef, !dbg !24
+  }
+
+  declare dso_local i8 @bar(i8, i8, i8*) #0
+
+  attributes #0 = { noinline nounwind optnone "frame-pointer"="all" }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+  !1 = !DIFile(filename: "wrong-callee-save-size-after-livedebugvariables.c", directory: "")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!"clang version 10.0.0"}
+  !7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10}
+  !10 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_unsigned_char)
+  !11 = !DILocalVariable(name: "obj1", scope: !7, file: !1, line: 4, type: !10)
+  !12 = !DILocation(line: 4, column: 8, scope: !7)
+  !13 = !DILocalVariable(name: "obj2", scope: !7, file: !1, line: 5, type: !10)
+  !14 = !DILocation(line: 5, column: 8, scope: !7)
+  !15 = !DILocalVariable(name: "obj3", scope: !7, file: !1, line: 6, type: !16)
+  !16 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, size: 1904, elements: !17)
+  !17 = !{!18}
+  !18 = !DISubrange(count: 238)
+  !19 = !DILocation(line: 6, column: 8, scope: !7)
+  !20 = !DILocation(line: 7, column: 14, scope: !7)
+  !21 = !DILocation(line: 7, column: 20, scope: !7)
+  !22 = !DILocation(line: 7, column: 27, scope: !7)
+  !23 = !DILocation(line: 7, column: 10, scope: !7)
+  !24 = !DILocation(line: 7, column: 3, scope: !7)
+
+...
+---
+name:            foo
+tracksRegLiveness: true
+frameInfo:
+  hasCalls:        true
+fixedStack:      []
+stack:
+  - { id: 0, name: obj1, type: default, offset: 0, size: 1, alignment: 1,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      local-offset: -1, debug-info-variable: '!11', debug-info-expression: '!DIExpression()',
+      debug-info-location: '!12' }
+  - { id: 1, name: obj2, type: default, offset: 0, size: 1, alignment: 1,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      local-offset: -2, debug-info-variable: '!13', debug-info-expression: '!DIExpression()',
+      debug-info-location: '!14' }
+  - { id: 2, name: obj3, type: default, offset: 0, size: 238, alignment: 1,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      local-offset: -240, debug-info-variable: '!15', debug-info-expression: '!DIExpression()',
+      debug-info-location: '!19' }
+body:             |
+  bb.1.entry:
+    renamable $x2 = ADDXri %stack.2.obj3, 0, 0
+    renamable $w0 = LDRBBui %stack.0.obj1, 0, debug-location !20 :: (load 1 from %ir.obj1)
+    renamable $w1 = LDRBBui %stack.1.obj2, 0, debug-location !21 :: (load 1 from %ir.obj2)
+    ADJCALLSTACKDOWN 0, 0, implicit-def $sp, implicit $sp, debug-location !23
+    BL @bar, csr_aarch64_aapcs, implicit-def $lr, implicit $sp, implicit killed $w0, implicit killed $w1, implicit killed $x2, implicit-def $w0, debug-location !23
+    ADJCALLSTACKUP 0, 0, implicit-def $sp, implicit $sp, debug-location !23
+    RET_ReallyLR implicit killed $w0, debug-location !24
+
+...