[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 ↗

(On Diff #218375)

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

llvm/lib/CodeGen/RegisterScavenging.cpp

802 ↗

(On Diff #218375)

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/lib/Target/AArch64/AArch64FrameLowering.cpp

void AArch64FrameLowering::determineCalleeSaves(MachineFunction &MF,
                                                 BitVector &SavedRegs,
                                                 RegScavenger *RS) const {
   // All calls are tail calls in GHC calling conv, and functions have no
   // prologue/epilogue.
   if (MF.getFunction().getCallingConv() == CallingConv::GHC)
     return;
 
   TargetFrameLowering::determineCalleeSaves(MF, SavedRegs, RS);
+
+  // If the callee-saved info has been calculated in a previous
+  // call to determineCalleeSaves, do not recalculate.
+  MachineFrameInfo &MFI = MF.getFrameInfo();
+  if (MFI.isCalleeSavedInfoValid()) {
+    for (CalleeSavedInfo &Info : MFI.getCalleeSavedInfo())
+      SavedRegs.set(Info.getReg());
+    return;
+  }
+
   const AArch64RegisterInfo *RegInfo = static_cast<const AArch64RegisterInfo *>(
       MF.getSubtarget().getRegisterInfo());
   AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
   unsigned UnspilledCSGPR = AArch64::NoRegister;
   unsigned UnspilledCSGPRPaired = AArch64::NoRegister;
 
-  MachineFrameInfo &MFI = MF.getFrameInfo();
   const MCPhysReg *CSRegs = MF.getRegInfo().getCalleeSavedRegs();
 
   unsigned BasePointerReg = RegInfo->hasBasePointer(MF)
                                 ? RegInfo->getBaseRegister()
                                 : (unsigned)AArch64::NoRegister;
 
   unsigned ExtraCSSpill = 0;
   // Figure out which callee-saved registers to save/restore.
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((!AFI->hasCalculatedCalleeSavedStackSize() ||
+          AFI->getCalleeSavedStackSize() == AlignedCSStackSize) &&
+         "Unexpected change to CalleeSavedStackSize");
+
   // 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/lib/Target/AArch64/AArch64MachineFunctionInfo.h

class AArch64FunctionInfo final : public MachineFunctionInfo {
   /// determineCalleeSaves().
   bool HasStackFrame = false;
 
   /// Amount of stack frame size, not including callee-saved registers.
   unsigned LocalStackSize;
 
   /// Amount of stack frame size used for saving callee-saved registers.
   unsigned CalleeSavedStackSize;
+  bool HasCalculatedCalleeSavedStackSize = false;
 
   /// Number of TLS accesses using the special (combinable)
   /// _TLS_MODULE_BASE_ symbol.
   unsigned NumLocalDynamicTLSAccesses = 0;
 
   /// FrameIndex for start of varargs area for arguments passed on the
   /// stack.
   int VarArgsStackIndex = 0;
public:
   }
 
   bool isSplitCSR() const { return IsSplitCSR; }
   void setIsSplitCSR(bool s) { IsSplitCSR = s; }
 
   void setLocalStackSize(unsigned Size) { LocalStackSize = Size; }
   unsigned getLocalStackSize() const { return LocalStackSize; }
 
-  void setCalleeSavedStackSize(unsigned Size) { CalleeSavedStackSize = Size; }
+  bool hasCalculatedCalleeSavedStackSize() const {
+    return HasCalculatedCalleeSavedStackSize;
+  }
+
+  void setCalleeSavedStackSize(unsigned Size) {
+    CalleeSavedStackSize = Size;
+    HasCalculatedCalleeSavedStackSize = true;
+  }
   unsigned getCalleeSavedStackSize() const { return CalleeSavedStackSize; }
 
   void incNumLocalDynamicTLSAccesses() { ++NumLocalDynamicTLSAccesses; }
   unsigned getNumLocalDynamicTLSAccesses() const {
     return NumLocalDynamicTLSAccesses;
   }
 
   Optional<bool> hasRedZone() const { return HasRedZone; }

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
+
+...