Improvements to call site register worklist

Summary:
This fixes PR44118. For cases where we have a chain like this:

  R8 = R1 (entry value)
  R0 = R8
  call @foo R0

the code that emits call site entries using entry values would not
follow that chain, instead emitting a call site entry with R8 as
location rather than R0. Such a case was discovered when originally
adding dbgcall-site-orr-moves.mir. This patch fixes that issue. This is
done by changing the ForwardedRegWorklist set to a map in which the
worklist registers always map to the parameter registers that they
describe.

Another thing this patch fixes is that worklist registers now can
describe more than one parameter register at a time. Such a case
occurred in dbgcall-site-interpretation.mir, resulting in a call site
entry not being emitted for one of the parameters.

Reviewers: djtodoro, NikolaPrica, aprantl, vsk

Reviewed By: vsk

Subscribers: hiraditya, llvm-commits

Tags: #debug-info, #llvm

Differential Revision: https://reviews.llvm.org/D73168

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

@@ -560,10 +560,38 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
   // Skip the call instruction.
   auto I = std::next(CallMI->getReverseIterator());
 
-  DenseSet<unsigned> ForwardedRegWorklist;
+  // Register worklist. Each register has an associated list of parameter
+  // registers whose call site values potentially can be described using that
+  // register.
+  using FwdRegWorklist = MapVector<unsigned, SmallVector<unsigned, 2>>;
+
+  FwdRegWorklist ForwardedRegWorklist;
+
+  // If an instruction defines more than one item in the worklist, we may run
+  // into situations where a worklist register's value is (potentially)
+  // described by the previous value of another register that is also defined
+  // by that instruction.
+  //
+  // This can for example occur in cases like this:
+  //
+  //   $r1 = mov 123
+  //   $r0, $r1 = mvrr $r1, 456
+  //   call @foo, $r0, $r1
+  //
+  // When describing $r1's value for the mvrr instruction, we need to make sure
+  // that we don't finalize an entry value for $r0, as that is dependent on the
+  // previous value of $r1 (123 rather than 456).
+  //
+  // In order to not have to distinguish between those cases when finalizing
+  // entry values, we simply postpone adding new parameter registers to the
+  // worklist, by first keeping them in this temporary container until the
+  // instruction has been handled.
+  FwdRegWorklist NewWorklistItems;
+
   // Add all the forwarding registers into the ForwardedRegWorklist.
   for (auto ArgReg : CallFwdRegsInfo->second) {
-    bool InsertedReg = ForwardedRegWorklist.insert(ArgReg.Reg).second;
+    bool InsertedReg =
+        ForwardedRegWorklist.insert({ArgReg.Reg, {ArgReg.Reg}}).second;
     assert(InsertedReg && "Single register used to forward two arguments?");
     (void)InsertedReg;
   }
@@ -574,12 +602,9 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
   // list, for which we do not describe a loaded value by
   // the describeLoadedValue(), we try to generate an entry value expression
   // for their call site value description, if the call is within the entry MBB.
-  // The RegsForEntryValues maps a forwarding register into the register holding
-  // the entry value.
   // TODO: Handle situations when call site parameter value can be described
   // as the entry value within basic blocks other than the first one.
   bool ShouldTryEmitEntryVals = MBB->getIterator() == MF->begin();
-  DenseMap<unsigned, unsigned> RegsForEntryValues;
 
   // If the MI is an instruction defining one or more parameters' forwarding
   // registers, add those defines.
@@ -592,23 +617,32 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
       if (MO.isReg() && MO.isDef() &&
           Register::isPhysicalRegister(MO.getReg())) {
         for (auto FwdReg : ForwardedRegWorklist)
-          if (TRI->regsOverlap(FwdReg, MO.getReg()))
-            Defs.insert(FwdReg);
+          if (TRI->regsOverlap(FwdReg.first, MO.getReg()))
+            Defs.insert(FwdReg.first);
       }
     }
   };
 
-  auto finishCallSiteParam = [&](DbgValueLoc DbgLocVal, unsigned Reg) {
-    unsigned FwdReg = Reg;
-    if (ShouldTryEmitEntryVals) {
-      auto EntryValReg = RegsForEntryValues.find(Reg);
-      if (EntryValReg != RegsForEntryValues.end())
-        FwdReg = EntryValReg->second;
+  auto finishCallSiteParams = [&](DbgValueLoc DbgLocVal,
+                                  ArrayRef<unsigned> ParamRegs) {
+    for (auto FwdReg : ParamRegs) {
+      DbgCallSiteParam CSParm(FwdReg, DbgLocVal);
+      Params.push_back(CSParm);
+      ++NumCSParams;
     }
+  };
 
-    DbgCallSiteParam CSParm(FwdReg, DbgLocVal);
-    Params.push_back(CSParm);
-    ++NumCSParams;
+  // Add Reg to the worklist, if it's not already present, and mark that the
+  // given parameter registers' values can (potentially) be described using
+  // that register.
+  auto addToWorklist = [](FwdRegWorklist &Worklist, unsigned Reg,
+                          ArrayRef<unsigned> ParamRegs) {
+    auto I = Worklist.insert({Reg, {}});
+    for (auto ParamReg : ParamRegs) {
+      assert(!is_contained(I.first->second, ParamReg) &&
+             "Same parameter described twice by forwarding reg");
+      I.first->second.push_back(ParamReg);
+    }
   };
 
   // Search for a loading value in forwarding registers.
@@ -632,17 +666,12 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
     if (FwdRegDefs.empty())
       continue;
 
-    // If the MI clobbers more then one forwarding register we must remove
-    // all of them from the working list.
-    for (auto Reg : FwdRegDefs)
-      ForwardedRegWorklist.erase(Reg);
-
     for (auto ParamFwdReg : FwdRegDefs) {
       if (auto ParamValue = TII->describeLoadedValue(*I, ParamFwdReg)) {
         if (ParamValue->first.isImm()) {
           int64_t Val = ParamValue->first.getImm();
           DbgValueLoc DbgLocVal(ParamValue->second, Val);
-          finishCallSiteParam(DbgLocVal, ParamFwdReg);
+          finishCallSiteParams(DbgLocVal, ForwardedRegWorklist[ParamFwdReg]);
         } else if (ParamValue->first.isReg()) {
           Register RegLoc = ParamValue->first.getReg();
           // TODO: For now, there is no use of describing the value loaded into the
@@ -657,16 +686,34 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
             DbgValueLoc DbgLocVal(ParamValue->second,
                                   MachineLocation(RegLoc,
                                                   /*IsIndirect=*/IsSPorFP));
-            finishCallSiteParam(DbgLocVal, ParamFwdReg);
+            finishCallSiteParams(DbgLocVal, ForwardedRegWorklist[ParamFwdReg]);
           // TODO: Add support for entry value plus an expression.
           } else if (ShouldTryEmitEntryVals &&
                      ParamValue->second->getNumElements() == 0) {
-            ForwardedRegWorklist.insert(RegLoc);
-            RegsForEntryValues[RegLoc] = ParamFwdReg;
+            assert(RegLoc != ParamFwdReg &&
+                   "Can't handle a register that is described by itself");
+            // ParamFwdReg was described by the non-callee saved register
+            // RegLoc. Mark that the call site values for the parameters are
+            // dependent on that register instead of ParamFwdReg. Since RegLoc
+            // may be a register that will be handled in this iteration, we
+            // postpone adding the items to the worklist, and instead keep them
+            // in a temporary container.
+            addToWorklist(NewWorklistItems, RegLoc,
+                          ForwardedRegWorklist[ParamFwdReg]);
           }
         }
       }
     }
+
+    // Remove all registers that this instruction defines from the worklist.
+    for (auto ParamFwdReg : FwdRegDefs)
+      ForwardedRegWorklist.erase(ParamFwdReg);
+
+    // Now that we are done handling this instruction, add items from the
+    // temporary worklist to the real one.
+    for (auto New : NewWorklistItems)
+      addToWorklist(ForwardedRegWorklist, New.first, New.second);
+    NewWorklistItems.clear();
   }
 
   // Emit the call site parameter's value as an entry value.
@@ -675,15 +722,8 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
     DIExpression *EntryExpr = DIExpression::get(
         MF->getFunction().getContext(), {dwarf::DW_OP_LLVM_entry_value, 1});
     for (auto RegEntry : ForwardedRegWorklist) {
-      unsigned FwdReg = RegEntry;
-      auto EntryValReg = RegsForEntryValues.find(RegEntry);
-        if (EntryValReg != RegsForEntryValues.end())
-          FwdReg = EntryValReg->second;
-
-      DbgValueLoc DbgLocVal(EntryExpr, MachineLocation(RegEntry));
-      DbgCallSiteParam CSParm(FwdReg, DbgLocVal);
-      Params.push_back(CSParm);
-      ++NumCSParams;
+      DbgValueLoc DbgLocVal(EntryExpr, MachineLocation(RegEntry.first));
+      finishCallSiteParams(DbgLocVal, RegEntry.second);
     }
   }
 }

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-orr-moves.mir

@@ -268,6 +268,5 @@ body:             |
 # CHECK-NEXT: DW_AT_abstract_origin (0x00000052 "call_int_int")
 #
 # CHECK: DW_TAG_GNU_call_site_parameter
-# FIXME: The DW_AT_location attribute should actually refer to W0! See PR44118.
-# CHECK-NEXT: DW_AT_location      (DW_OP_reg8 W8)
+# CHECK-NEXT: DW_AT_location      (DW_OP_reg0 W0)
 # CHECK-NEXT: DW_AT_GNU_call_site_value   (DW_OP_GNU_entry_value(DW_OP_reg1 W1))

llvm/test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir

@@ -13,6 +13,10 @@
 # CHECK-NEXT:       DW_AT_GNU_call_site_value   (DW_OP_fbreg +8)
 # CHECK-EMPTY: 
 # CHECK-NEXT:     DW_TAG_GNU_call_site_parameter
+# CHECK-NEXT:       DW_AT_location      (DW_OP_reg4 RSI)
+# CHECK-NEXT:       DW_AT_GNU_call_site_value   (DW_OP_GNU_entry_value(DW_OP_reg4 RSI))
+# CHECK-EMPTY: 
+# CHECK-NEXT:     DW_TAG_GNU_call_site_parameter
 # CHECK-NEXT:       DW_AT_location      (DW_OP_reg5 RDI)
 # CHECK-NEXT:       DW_AT_GNU_call_site_value   (DW_OP_GNU_entry_value(DW_OP_reg4 RSI))
 # CHECK-EMPTY:

llvm/test/DebugInfo/MIR/X86/dbgcall-site-partial-describe.mir

@@ -0,0 +1,82 @@
+# RUN: llc -debug-entry-values -start-before=livedebugvalues -filetype=obj -o - %s \
+# RUN:     | llvm-dwarfdump - | FileCheck %s --implicit-check-not=DW_TAG_GNU_call_site_parameter
+
+--- |
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+
+  ; Function Attrs: nounwind uwtable
+  define void @foo() #0 !dbg !12 {
+  entry:
+    call void @call(i32 123, i32 undef), !dbg !15
+    ret void, !dbg !16
+  }
+
+  declare !dbg !4 void @call(i32, i32)
+
+  attributes #0 = { nounwind uwtable }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!8, !9, !10}
+  !llvm.ident = !{!11}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "clobber.c", directory: "/")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "call", scope: !1, file: !1, line: 1, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{null, !7, !7}
+  !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !8 = !{i32 7, !"Dwarf Version", i32 4}
+  !9 = !{i32 2, !"Debug Info Version", i32 3}
+  !10 = !{i32 1, !"wchar_size", i32 4}
+  !11 = !{!"clang version 11.0.0"}
+  !12 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !13, scopeLine: 3, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+  !13 = !DISubroutineType(types: !14)
+  !14 = !{null}
+  !15 = !DILocation(line: 5, scope: !12)
+  !16 = !DILocation(line: 6, scope: !12)
+
+...
+---
+name:            foo
+callSites:
+  - { bb: 0, offset: 4, fwdArgRegs:
+      - { arg: 0, reg: '$edi' }
+      - { arg: 1, reg: '$esi' } }
+body:             |
+  bb.0.entry:
+    frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    $esi = MOV32ri 123, debug-location !15
+    $edi = MOV32rr $esi, implicit-def $esi, debug-location !15
+    CALL64pcrel32 @call, csr_64, implicit $rsp, implicit $ssp, implicit killed $edi, implicit undef $esi, implicit-def $rsp, implicit-def $ssp, debug-location !15
+    $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !16
+    CFI_INSTRUCTION def_cfa_offset 8, debug-location !16
+    RETQ debug-location !16
+
+...
+
+# In this test an implicit-def has been added to the MOV32rr instruction to
+# simulate a situation where one instruction defines two call site worklist
+# registers, and where we end up with one of the registers being described by
+# the previous value of the other register that is clobbered by this
+# instruction.
+#
+# In this reproducer we should end up with only one call site entry, with that
+# being for $rdi.
+#
+# This test uses an implicit CHECK-NOT to verify that only one call site
+# parameter entry is emitted.
+#
+# A somewhat more realistic scenario would for example be the following in a
+# made-up ISA:
+#
+# $r1 = mv 123
+# $r0, $r1 = mvri $r1, <undescribable value>
+# call @foo, $r0, $r1
+
+# CHECK: DW_TAG_GNU_call_site_parameter
+# CHECK-NEXT: DW_AT_location	(DW_OP_reg5 RDI)
+# CHECK-NEXT: DW_AT_GNU_call_site_value	(DW_OP_constu 0x7b)

llvm/test/DebugInfo/MIR/X86/dbgcall-site-reg-shuffle.mir

@@ -0,0 +1,93 @@
+# RUN: llc -debug-entry-values -start-before=livedebugvalues -filetype=obj -o - %s \
+# RUN:     | llvm-dwarfdump - | FileCheck %s --implicit-check-not=DW_TAG_GNU_call_site_parameter
+
+--- |
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+
+  ; Function Attrs: nounwind uwtable
+  define void @move_around_args(i32 %a) #0 !dbg !12 {
+  entry:
+    call void @call2(i32 123, i32 %a), !dbg !15
+    ret void, !dbg !16
+  }
+
+  declare !dbg !4 dso_local void @call2(i32, i32)
+
+  attributes #0 = { nounwind uwtable }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!8, !9, !10}
+  !llvm.ident = !{!11}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "worklist.c", directory: "/")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "call2", scope: !1, file: !1, line: 1, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{null, !7, !7}
+  !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !8 = !{i32 7, !"Dwarf Version", i32 4}
+  !9 = !{i32 2, !"Debug Info Version", i32 3}
+  !10 = !{i32 1, !"wchar_size", i32 4}
+  !11 = !{!"clang version 11.0.0"}
+  !12 = distinct !DISubprogram(name: "move_around_args", scope: !1, file: !1, line: 3, type: !13, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+  !13 = !DISubroutineType(types: !14)
+  !14 = !{null, !7}
+  !15 = !DILocation(line: 4, scope: !12)
+  !16 = !DILocation(line: 5, scope: !12)
+
+...
+---
+name:            move_around_args
+liveins:
+  - { reg: '$edi' }
+callSites:
+  - { bb: 0, offset: 12, fwdArgRegs:
+      - { arg: 0, reg: '$edi' }
+      - { arg: 1, reg: '$esi' } }
+body:             |
+  bb.0.entry:
+    liveins: $edi
+
+    frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    $esi = MOV32ri 123
+
+    ; Move the values around between different registers.
+
+    $edx = MOV32rr $edi
+
+    $edi = MOV32rr $esi
+    $esi = MOV32rr $edx
+
+    $edx = MOV32rr $edi
+    $eax = MOV32rr $esi
+
+    $esi = MOV32rr $edx
+    $edx = MOV32rr $eax
+
+    $edi = MOV32rr $esi
+    $esi = MOV32rr $edx
+
+    CALL64pcrel32 @call2, csr_64, implicit $rsp, implicit $ssp, implicit $edi, implicit $esi, implicit-def $rsp, implicit-def $ssp, debug-location !15
+    $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !16
+    CFI_INSTRUCTION def_cfa_offset 8, debug-location !16
+    RETQ debug-location !16
+
+...
+
+# Verify that we emit correct call site parameter entries even after moving
+# around the call site values between different registers.
+#
+# This test uses an implicit CHECK-NOT to verify that only two call site
+# parameter entries are emitted.
+
+# CHECK: DW_TAG_GNU_call_site_parameter
+# CHECK-NEXT: DW_AT_location	(DW_OP_reg5 RDI)
+# CHECK-NEXT: DW_AT_GNU_call_site_value	(DW_OP_constu 0x7b)
+
+# CHECK: DW_TAG_GNU_call_site_parameter
+# CHECK-NEXT: DW_AT_location	(DW_OP_reg4 RSI)
+# CHECK-NEXT: DW_AT_GNU_call_site_value	(DW_OP_GNU_entry_value(DW_OP_reg5 RDI))

llvm/test/DebugInfo/MIR/X86/dbgcall-site-two-fwd-reg-defs.mir

@@ -132,10 +132,11 @@ body:             |
 # and the value for the first parameter (passed in $rdi) should be
 # sign-extended to 64 bits.
 
+# CHECK: DW_TAG_call_site_parameter
+# CHECK-NEXT: DW_AT_location      (DW_OP_reg5 RDI)
+# CHECK-NEXT: DW_AT_call_value    (DW_OP_breg3 RBX+0, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_convert ({{.*}}) "DW_ATE_signed_32", DW_OP_convert ({{.*}}) "DW_ATE_signed_64")
+#
 # CHECK: DW_TAG_call_site_parameter
 # CHECK-NEXT: DW_AT_location      (DW_OP_reg4 RSI)
 # CHECK-NEXT: DW_AT_call_value    (DW_OP_breg3 RBX+0)
 
-# CHECK: DW_TAG_call_site_parameter
-# CHECK-NEXT: DW_AT_location      (DW_OP_reg5 RDI)
-# CHECK-NEXT: DW_AT_call_value    (DW_OP_breg3 RBX+0, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_convert ({{.*}}) "DW_ATE_signed_32", DW_OP_convert ({{.*}}) "DW_ATE_signed_64")