[DebugInfo] Insert DEBUG_VALUEs after each register redefinition

[Ka-Ka]

Summary

When reinserting debug values after register allocation, make sure to
insert debug values after each redefinition of debug value register in
the slot index range. The reason for this is that DwarfDebug will end
the range of a debug variable when the physical reg is defined. For
instructions with e.g. tied operands this result in prematurely ended
debug range.

This resolves pr34545

Patch by Karl-Johan Karlsson and Bjorn Pettersson

[rnk]

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

[aprantl]

In D38229#880149, @rnk wrote:

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

In LiveDebugValues we use a heuristic based on the lexical scopes to avoid inserting DBG_VALUEs long after the variable went out of scope. Perhaps a similar approach is useful here, too? Otherwise, using the DebugLoc is even stricter, so that's fine with me, too.

lib/CodeGen/LiveDebugVariables.cpp

1039

Please remove the findNextInsertLocation - we don't do/need this any more.

1058

return std::next(I); ? (not sure if this is better :-)

1071

.

[Ka-Ka]

In D38229#880149, @rnk wrote:

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

I agree that we need to be careful not to introduce faulty debug info, but I don't really see how this patch could introduce inaccurate debug info at least not for the testcase you specified. The patch will only reinsert DBG_VALUES when redefining the vreg within the vregs liverange (indicated by the slot indexes). Note that the patch only try to reinsert all values that was originally found in the input before they were removed by the "Live DEBUG_VALUE analysis" pass. No hoisting of DBG_VALUEs are done. As the liverange of the physical registers are changed to not be extended in the patch in D38140 (a prerequisite to this patch) no extra DEBUG_VALUEs will be inserted for physical registers.

When I tried your test program above I found that faulty hoisting of a DBG_VALUE across the call to usevals seems to be done by the CodeGenPrepare pass (before instruction selection). This must be a bug. If you want I can convert code above into a testcase but it will contain this faulty hoisted DBG_VALUE from the CodeGenPrepare pass. This patch will not impact that testcase.

[Ka-Ka]

Minor fixes.

[Ka-Ka]

When I tried your test program above I found that faulty hoisting of a DBG_VALUE across the call to usevals seems to be done by the CodeGenPrepare pass (before instruction selection). This must be a bug. If you want I can convert code above into a testcase but it will contain this faulty hoisted DBG_VALUE from the CodeGenPrepare pass. This patch will not impact that testcase.

There already exist an old bug report regarding CodeGenPrepare moving llvm.dbg.value without proper analysis. I appended a new reproducer to pr31878.
https://bugs.llvm.org/show_bug.cgi?id=31878

[Ka-Ka]

In D38229#880149, @rnk wrote:

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

I guess this is the case that you have concern about. Note that this is not actually how the input mir to greedy look like when running the code through llc (due to pr31878). I have edited the mir before feeding it to greedy to get the case that I think you are concerned about.

bb.0.entry:
  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !14
  dead %eax = MOV32r0 implicit-def dead %eflags, implicit-def %al, debug-location !14
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit killed %al, implicit-def %rsp, implicit-def %eax, debug-location !14
  ADJCALLSTACKUP64 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !14
  %2 = COPY killed %eax, debug-location !14
  DBG_VALUE debug-use %2, debug-use _, !11, !DIExpression(), debug-location !15
  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !16
  dead %eax = MOV32r0 implicit-def dead %eflags, implicit-def %al, debug-location !16
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit killed %al, implicit-def %rsp, implicit-def %eax, debug-location !16
  ADJCALLSTACKUP64 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !16
  %3 = COPY killed %eax, debug-location !16
  DBG_VALUE debug-use %3, debug-use _, !13, !DIExpression(), debug-location !17
  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !18
  %edi = COPY %2, debug-location !18
  %esi = COPY %3, debug-location !18
  CALL64pcrel32 @usevals, csr_64, implicit %rsp, implicit killed %edi, implicit killed %esi, implicit-def %rsp, debug-location !18
  DBG_VALUE debug-use %3, debug-use _, !11, !DIExpression(), debug-location !15
  ADJCALLSTACKUP64 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !18
  RET 0, debug-location !19

When looking at the output after livedebugvariables it look like this:

bb.0.entry:
  liveins: %rbx

  frame-setup PUSH64r killed %rbx, implicit-def %rsp, implicit %rsp, debug-location !14
  CFI_INSTRUCTION def_cfa_offset 16
  CFI_INSTRUCTION offset %rbx, -16
  dead %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags, implicit-def %al, debug-location !14
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit %al, implicit-def %rsp, implicit-def %eax, debug-location !14
  %ebx = MOV32rr %eax, debug-location !14
  DBG_VALUE debug-use %ebx, debug-use _, !11, !DIExpression(), debug-location !15
  dead %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags, implicit-def %al, debug-location !16
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit %al, implicit-def %rsp, implicit-def %eax, debug-location !16
  DBG_VALUE debug-use %eax, debug-use _, !13, !DIExpression(), debug-location !17
  %edi = MOV32rr killed %ebx, debug-location !18
  %esi = MOV32rr killed %eax, debug-location !18
  CALL64pcrel32 @usevals, csr_64, implicit %rsp, implicit killed %edi, implicit killed %esi, implicit-def %rsp, debug-location !18
  DBG_VALUE debug-use %eax, debug-use _, !11, !DIExpression(), debug-location !15
  %rbx = POP64r implicit-def %rsp, implicit %rsp, debug-location !19
  RETQ debug-location !19

The last DBG_VALUE is wrong:

DBG_VALUE debug-use %eax, debug-use _, !11, !DIExpression(), debug-location !15

As I understand your comment this is the situation you are worried about. The value in %eax do not contain any value connected to the variable the DBG_VALUE is suppose to describe as the range ended before the call to usevals. The register allocator might have reused eax for another value.

I agree this is a fault, but the output from livedebugvariables shown above is from what is already commited on trunk in svn. The output looks exactly the same with or without the patch in this review. Yes, this is a fault but it is another fault not related to this patch.

[aprantl]

Could you please file a PR about this?

[Ka-Ka]

In D38229#884660, @aprantl wrote:

Could you please file a PR about this?

Done
https://bugs.llvm.org/show_bug.cgi?id=34788

[Ka-Ka]

Rebased

[rnk]

lgtm, my concern about hoisting can definitely be handled separately.

lib/CodeGen/LiveDebugVariables.cpp

1071

These slot indices never cross BBs, right? We should be pre-layout here, so it wouldn't make sense to try to coalesce variable locations across MBBs.

test/DebugInfo/X86/pr34545.ll

27

You know, we really should create an LLVM intrinsic for this. I would like a target-neutral way to emit a full register mask.

lib/CodeGen/LiveDebugVariables.cpp

class UserValue {
   /// Map of slot indices where this value is live.
   LocMap locInts;
 
   /// Set of interval start indexes that have been trimmed to the
   /// lexical scope.
   SmallSet<SlotIndex, 2> trimmedDefs;
 
   /// insertDebugValue - Insert a DBG_VALUE into MBB at Idx for LocNo.
-  void insertDebugValue(MachineBasicBlock *MBB, SlotIndex Idx,
+  void insertDebugValue(MachineBasicBlock *MBB, SlotIndex StartIdx,
+                        SlotIndex StopIdx,
                         unsigned LocNo, bool Spilled, LiveIntervals &LIS,
-                        const TargetInstrInfo &TII);
+                        const TargetInstrInfo &TII,
+                        const TargetRegisterInfo &TRI);
 
   /// splitLocation - Replace OldLocNo ranges with NewRegs ranges where NewRegs
   /// is live. Returns true if any changes were made.
   bool splitLocation(unsigned OldLocNo, ArrayRef<unsigned> NewRegs,
                      LiveIntervals &LIS);
 
 public:
   /// UserValue - Create a new UserValue.
public:
 
   /// rewriteLocations - Rewrite virtual register locations according to the
   /// provided virtual register map. Record which locations were spilled.
   void rewriteLocations(VirtRegMap &VRM, const TargetRegisterInfo &TRI,
                         BitVector &SpilledLocations);
 
   /// emitDebugValues - Recreate DBG_VALUE instruction from data structures.
   void emitDebugValues(VirtRegMap *VRM, LiveIntervals &LIS,
-                       const TargetInstrInfo &TRI,
+                       const TargetInstrInfo &TII,
+                       const TargetRegisterInfo &TRI,
                        const BitVector &SpilledLocations);
 
   /// getDebugLoc - Return DebugLoc of this UserValue.
   DebugLoc getDebugLoc() { return dl;}
 
   void print(raw_ostream &, const TargetRegisterInfo *);
 };
 
while (!(MI = LIS.getInstructionFromIndex(Idx))) {
     Idx = Idx.getPrevIndex();
   }
 
   // Don't insert anything after the first terminator, though.
   return MI->isTerminator() ? MBB->getFirstTerminator() :
                               std::next(MachineBasicBlock::iterator(MI));
 }
 
-void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex Idx,
+/// findNextInsertLocation - Find an iterator for inserting the next DBG_VALUE
+/// instruction (or end if no more insert locations found).
+static MachineBasicBlock::iterator
+findNextInsertLocation(MachineBasicBlock *MBB,
+                       MachineBasicBlock::iterator I,
+                       SlotIndex StopIdx, MachineOperand &Loc,
+                       LiveIntervals &LIS,
+                       const TargetRegisterInfo &TRI) {
+  if (!Loc.isReg())
+    return MBB->instr_end();
+  unsigned Reg = Loc.getReg();
+
+  // Find the next instruction in the MBB that define the register Reg
+  while (I != MBB->end()) {
+    if (!LIS.isNotInMIMap(*I) &&
+        SlotIndex::isEarlierEqualInstr(StopIdx, LIS.getInstructionIndex(*I)))
+      break;
+    if (I->definesRegister(Reg, &TRI))
+      // The insert location is directly after the instruction/bundle
+      return ++I;
+    ++I;
+  }
+  return MBB->end();
+}
+
+void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex StartIdx,
+                                 SlotIndex StopIdx,
                                  unsigned LocNo, bool Spilled,
                                  LiveIntervals &LIS,
-                                 const TargetInstrInfo &TII) {
-  MachineBasicBlock::iterator I = findInsertLocation(MBB, Idx, LIS);
+                                 const TargetInstrInfo &TII,
+                                 const TargetRegisterInfo &TRI) {
+  SlotIndex MBBEndIdx = LIS.getMBBEndIdx(&*MBB);
+  // Only search within the current MBB
+  StopIdx = (MBBEndIdx < StopIdx) ? MBBEndIdx : StopIdx;
+  MachineBasicBlock::iterator I = findInsertLocation(MBB, StartIdx, LIS);
   MachineOperand &Loc = locations[LocNo];
   ++NumInsertedDebugValues;
 
   assert(cast<DILocalVariable>(Variable)
              ->isValidLocationForIntrinsic(getDebugLoc()) &&
          "Expected inlined-at fields to agree");
 
   // If the location was spilled, the new DBG_VALUE will be indirect. If the
   // original DBG_VALUE was indirect, we need to add DW_OP_deref to indicate
   // that the original virtual register was a pointer.
   bool NewIndirect = IsIndirect || Spilled;
   const DIExpression *Expr = Expression;
   if (Spilled && IsIndirect)
     Expr = DIExpression::prepend(Expr, DIExpression::WithDeref);
 
   assert((!Spilled || Loc.isFI()) &&
          "a spilled location must be a frame index");
 
+  do {
     MachineInstrBuilder MIB =
       BuildMI(*MBB, I, getDebugLoc(), TII.get(TargetOpcode::DBG_VALUE))
           .add(Loc);
     if (NewIndirect)
       MIB.addImm(0U);
     else
       MIB.addReg(0U, RegState::Debug);
     MIB.addMetadata(Variable).addMetadata(Expr);
+
+    // Continue and insert DBG_VALUES after every redefinition of register
+    // associated with the debug value within the range
+    I = findNextInsertLocation(MBB, I, StopIdx, Loc, LIS, TRI);
+  } while (I != MBB->end());
 }
 
 void UserValue::emitDebugValues(VirtRegMap *VRM, LiveIntervals &LIS,
                                 const TargetInstrInfo &TII,
+                                const TargetRegisterInfo &TRI,
                                 const BitVector &SpilledLocations) {
   MachineFunction::iterator MFEnd = VRM->getMachineFunction().end();
 
   for (LocMap::const_iterator I = locInts.begin(); I.valid();) {
     SlotIndex Start = I.start();
     SlotIndex Stop = I.stop();
     unsigned LocNo = I.value();
     bool Spilled = LocNo != UndefLocNo ? SpilledLocations.test(LocNo) : false;
 
     // If the interval start was trimmed to the lexical scope insert the
     // DBG_VALUE at the previous index (otherwise it appears after the
     // first instruction in the range).
     if (trimmedDefs.count(Start))
       Start = Start.getPrevIndex();
 
     DEBUG(dbgs() << "\t[" << Start << ';' << Stop << "):" << LocNo);
     MachineFunction::iterator MBB = LIS.getMBBFromIndex(Start)->getIterator();
     SlotIndex MBBEnd = LIS.getMBBEndIdx(&*MBB);
 
     DEBUG(dbgs() << " BB#" << MBB->getNumber() << '-' << MBBEnd);
-    insertDebugValue(&*MBB, Start, LocNo, Spilled, LIS, TII);
+    insertDebugValue(&*MBB, Start, Stop, LocNo, Spilled, LIS, TII, TRI);
     // This interval may span multiple basic blocks.
     // Insert a DBG_VALUE into each one.
-    while(Stop > MBBEnd) {
+    while (Stop > MBBEnd) {
       // Move to the next block.
       Start = MBBEnd;
       if (++MBB == MFEnd)
         break;
       MBBEnd = LIS.getMBBEndIdx(&*MBB);
       DEBUG(dbgs() << " BB#" << MBB->getNumber() << '-' << MBBEnd);
-      insertDebugValue(&*MBB, Start, LocNo, Spilled, LIS, TII);
+      insertDebugValue(&*MBB, Start, Stop, LocNo, Spilled, LIS, TII, TRI);
     }
     DEBUG(dbgs() << '\n');
     if (MBB == MFEnd)
       break;
 
     ++I;
   }
 }
 
 void LDVImpl::emitDebugValues(VirtRegMap *VRM) {
   DEBUG(dbgs() << "********** EMITTING LIVE DEBUG VARIABLES **********\n");
   if (!MF)
     return;
   const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
   BitVector SpilledLocations;
   for (unsigned i = 0, e = userValues.size(); i != e; ++i) {
     DEBUG(userValues[i]->print(dbgs(), TRI));
     userValues[i]->rewriteLocations(*VRM, *TRI, SpilledLocations);
-    userValues[i]->emitDebugValues(VRM, *LIS, *TII, SpilledLocations);
+    userValues[i]->emitDebugValues(VRM, *LIS, *TII, *TRI, SpilledLocations);
   }
   EmitDone = true;
 }
 
 void LiveDebugVariables::emitDebugValues(VirtRegMap *VRM) {
   if (pImpl)
     static_cast<LDVImpl*>(pImpl)->emitDebugValues(VRM);
 }

test/DebugInfo/X86/pr34545.ll

+; RUN: llc -O1 -filetype=asm -mtriple x86_64-unknown-linux-gnu -mcpu=x86-64 -o - %s -stop-after=livedebugvars | FileCheck %s
+
+; CHECK: %eax = MOV32rm
+; CHECK: DBG_VALUE %eax
+; CHECK: %eax = SHL32rCL killed %eax
+; CHECK: DBG_VALUE %eax
+; CHECK: DBG_VALUE %rsp, 0, !{{[0-9]+}}, !DIExpression(DW_OP_constu, 4, DW_OP_minus)
+; CHECK: DBG_VALUE %eax
+; CHECK: %eax = SHL32rCL killed %eax
+; CHECK: DBG_VALUE %eax
+; CHECK: RETQ %eax
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@var = local_unnamed_addr global i32 8, !dbg !0
+@sc = local_unnamed_addr global i32 1, !dbg !6
+
+define i32 @main() local_unnamed_addr !dbg !14 {
+entry:
+  %0 = load i32, i32* @var
+  tail call void @llvm.dbg.value(metadata i32 %0, metadata !18, metadata !DIExpression()), !dbg !20
+  %1 = load i32, i32* @sc
+  %shl = shl i32 %0, %1
+  tail call void @llvm.dbg.value(metadata i32 %shl, metadata !18, metadata !DIExpression()), !dbg !20
+  tail call void asm sideeffect "", "~{rax},~{rbx},~{rcx},~{rdx},~{rsi},~{rdi},~{rbp},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"(), !srcloc !25
+  %2 = load i32, i32* @sc
+  %shl2 = shl i32 %shl, %2
+  tail call void @llvm.dbg.value(metadata i32 %shl2, metadata !18, metadata !DIExpression()), !dbg !20
+  store i32 %shl2, i32* @var
+  ret i32 %shl2
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!10, !11, !12}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "var", scope: !2, file: !3, line: 10, type: !9, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 6.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5)
+!3 = !DIFile(filename: "bar.c", directory: ".")
+!4 = !{}
+!5 = !{!0, !6}
+!6 = !DIGlobalVariableExpression(var: !7, expr: !DIExpression())
+!7 = distinct !DIGlobalVariable(name: "sc", scope: !2, file: !3, line: 11, type: !8, isLocal: false, isDefinition: true)
+!8 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!10 = !{i32 2, !"Dwarf Version", i32 4}
+!11 = !{i32 2, !"Debug Info Version", i32 3}
+!12 = !{i32 1, !"wchar_size", i32 4}
+!14 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 12, type: !15, isLocal: false, isDefinition: true, scopeLine: 12, isOptimized: true, unit: !2, variables: !17)
+!15 = !DISubroutineType(types: !16)
+!16 = !{!9}
+!17 = !{!18}
+!18 = !DILocalVariable(name: "bazinga", scope: !14, file: !3, line: 13, type: !9)
+!20 = !DILocation(line: 13, column: 7, scope: !14)
+!25 = !{i32 -2147471481}