[DebugInfo][Dexter] Incorrect DBG_VALUE after MCP dead copy instruction removal.

[CarlosAlbertoEnciso]

Summary

For the below test:

int main() {
  volatile int foo = 4;
  int read1 = foo;
  int read2 = foo;

  switch ((read1 == 4) ? 3 : 1) {
  case 1:
    read1 *= read2;
    break;
  case 3:
    read1 /= read2;
    break;
  }

  return read1;
}

Debug location information for 'read1' is wrong with -O2.

When in the debugger, on the line "return read1;", the value of "read1"
is reported as '4', where it should be '1', as described by:

PR38773

[aprantl]

Thanks, this looks mostly good!

include/llvm/CodeGen/MachineInstr.h

1547 ↗

(On Diff #167325)

How about:

/// Find all DBG_VALUEs immediately following this instruction that point to a register def in this instruction and point them to \p Reg instead.

Looking at the implementation of collectDebugValues() I'm somewhat puzzled how it works. Perhaps I'm misunderstanding what it is supposed to do, please let me know. It's scanning for DBG_VALUEs that point to Op0 of the instruction. Is Op0 always going to be a def? Or should it be improved?

1548 ↗

(On Diff #167325)

Could you give this a more descriptive name?

test/CodeGen/MIR/X86/pr38773.mir

153 ↗

(On Diff #167325)

This line is an exact duplicate of the previous line. While this won't break anything, it's inefficient.
Looking at the definition of collectDebugValues it doesn't look like this patch is responsible, but it would still be good to fix. Whoever is inserting these DBG_VALUEs in the first place should be doing a check for duplicates? Doing a linear search of any DBG_VALUEs immediately following the insertion point (like collectDebugValues does) should be sufficient.

[dstenb]

It would be good to change the title to reflect what is done in MCP (at least before submitting this).

Can the test be reduced to only contain the bb.2.sw.bb1 block?

test/CodeGen/MIR/X86/pr38773.mir

46 ↗

(On Diff #167325)

Can the lifetime markers be removed?

50 ↗

(On Diff #167325)

Should unrelated debug values for other variables be removed to minimize the test?

[CarlosAlbertoEnciso]

In D52614#1248058, @dstenb wrote:

It would be good to change the title to reflect what is done in MCP (at least before submitting this).

We are using the tags: [DebugInfo][Dexter] and identify the issues detected by the DexTer tool.

For the title, may be something like:

Incorrect DBG_VALUE after MCP dead copy instruction removal.

Can the test be reduced to only contain the bb.2.sw.bb1 block?

Yes. I have reduced the test case, just to contain bb.2.sw.bb1 block.

include/llvm/CodeGen/MachineInstr.h

1547 ↗

(On Diff #167325)

/// Find all DBG_VALUEs immediately following this instruction that point to a register def in this instruction and point them to \p Reg instead.

That is a better description.

test/CodeGen/MIR/X86/pr38773.mir

46 ↗

(On Diff #167325)

I have removed all the not required markers.

50 ↗

(On Diff #167325)

I have removed all the not required debug values. I have left only the ones for 'read1'.

153 ↗

(On Diff #167325)

The duplicated line has been removed. Modified the IR just to include only a single DBG_VALUE.

153 ↗

(On Diff #167325)

Looking at the definition of collectDebugValues it doesn't look like this patch is responsible, but it would still be good to fix.

If you are OK, I can leave that issue to be fix in another patch.

Whoever is inserting these DBG_VALUEs in the first place should be doing a check for duplicates? Doing a linear search of any DBG_VALUEs immediately following the insertion point (like collectDebugValues does) should be sufficient.

That is a very good point.

During the review D50887, there were some discussions about the collectDebugValues implementation. The idea was to use the list of USEs instead of linearly scanning.

Another interesting modification, was the one you raised on PR38763#c8 to extended llvm.dbg.value to take more than one LLVM SSA Value.

I would suggest if we can create specific bugzillas to address the following issues:

• collectDebugValues implementation (to use the list of USEs).
• Check for duplicates when inserting DBG_VALUEs.
• Extended llvm.dbg.value to take more than one LLVM SSA Value.

I am happy to create those bugzillas, so we can have something in concrete to work on.

[CarlosAlbertoEnciso]

include/llvm/CodeGen/MachineInstr.h

1547 ↗

(On Diff #167325)

Looking at the implementation of collectDebugValues() I'm somewhat puzzled how it works. Perhaps I'm misunderstanding what it is supposed to do, please let me know. It's scanning for DBG_VALUEs that point to Op0 of the instruction.

It scans all the MI that follows the given instruction (they are referenced by 'DI').

if (!DI->isDebugValue())
  return;

Returns when the scanned DI is not a DBG_VALUE

if (DI->getOperand(0).isReg() &&
    DI->getOperand(0).getReg() == MI.getOperand(0).getReg())
  DbgValues.push_back(&*DI);

Records the scanned DI (which already we know is DBG_VALUE), only if:

• The DI machine operand type is a 'register operand' and
• The DI register number is the same as the MI register number.

Is Op0 always going to be a def?

As far as I understand it, only for the case of DBG_VALUE, the Op0 should always be a def.

Or should it be improved?

Only the first operand in the MI (getOperand(0)) is considered, which I can see as a possible limitation for cases where a different operand register needs to be modified.

And if the assumption that Op0 is not always a def, then extra checks should be added.

1548 ↗

(On Diff #167325)

What about: changeDebugValuesDefReg

meaning

Changes the register definition for the DBG_VALUEs to the given 'register'.

[CarlosAlbertoEnciso]

Address issues raised by the reviewers.

• Title for the review itself
• Reduce test case
• Better description and name for the new function.

[aprantl]

include/llvm/CodeGen/MachineInstr.h

1547 ↗

(On Diff #167325)

Oh I see. MI is always a DBG_VALUE. Then looking at Op0 makes perfect sense. Thanks for clearing that up!

test/CodeGen/MIR/X86/pr38773.mir

153 ↗

(On Diff #167325)

I am happy to create those bugzillas, so we can have something in concrete to work on.

Please do. Working in small increments makes it much easier to reason over patches and also makes reviews much faster!

[vsk]

+ 1, lgtm as well, thanks!

[CarlosAlbertoEnciso]

Thanks very much to all the reviewers for your valuable comments and suggestions.

llvm/trunk/include/llvm/CodeGen/MachineInstr.h

   }
 
   /// Add all implicit def and use operands to this instruction.
   void addImplicitDefUseOperands(MachineFunction &MF);
 
   /// Scan instructions following MI and collect any matching DBG_VALUEs.
   void collectDebugValues(SmallVectorImpl<MachineInstr *> &DbgValues);
 
+  /// Find all DBG_VALUEs immediately following this instruction that point
+  /// to a register def in this instruction and point them to \p Reg instead.
+  void changeDebugValuesDefReg(unsigned Reg);
+
 private:
   /// If this instruction is embedded into a MachineFunction, return the
   /// MachineRegisterInfo object for the current function, otherwise
   /// return null.
   MachineRegisterInfo *getRegInfo();
 
   /// Unlink all of the register operands in this instruction from their
   /// respective use lists.  This requires that the operands already be on their

llvm/trunk/lib/CodeGen/MachineCSE.cpp

for (MachineOperand &MO : MI->operands()) {
     // class given a super-reg class and subreg index.
     if (DefMI->getOperand(1).getSubReg())
       continue;
     if (!MRI->constrainRegAttrs(SrcReg, Reg))
       continue;
     LLVM_DEBUG(dbgs() << "Coalescing: " << *DefMI);
     LLVM_DEBUG(dbgs() << "***     to: " << *MI);
 
-    // Collect matching debug values.
-    SmallVector<MachineInstr *, 2> DbgValues;
-    DefMI->collectDebugValues(DbgValues);
-    // Propagate SrcReg to debug value instructions.
-    for (auto *DBI : DbgValues)
-      DBI->getOperand(0).setReg(SrcReg);
+    // Update matching debug values.
+    DefMI->changeDebugValuesDefReg(SrcReg);
 
     // Propagate SrcReg of copies to MI.
     MO.setReg(SrcReg);
     MRI->clearKillFlags(SrcReg);
     // Coalesce single use copies.
     if (OnlyOneUse) {
       DefMI->eraseFromParent();
       ++NumCoalesces;

llvm/trunk/lib/CodeGen/MachineCopyPropagation.cpp

void MachineCopyPropagation::CopyPropagateBlock(MachineBasicBlock &MBB) {
   // If MBB doesn't have successors, delete the copies whose defs are not used.
   // If MBB does have successors, then conservative assume the defs are live-out
   // since we don't want to trust live-in lists.
   if (MBB.succ_empty()) {
     for (MachineInstr *MaybeDead : MaybeDeadCopies) {
       LLVM_DEBUG(dbgs() << "MCP: Removing copy due to no live-out succ: ";
                  MaybeDead->dump());
       assert(!MRI->isReserved(MaybeDead->getOperand(0).getReg()));
+
+      // Update matching debug values.
+      assert(MaybeDead->isCopy());
+      MaybeDead->changeDebugValuesDefReg(MaybeDead->getOperand(1).getReg());
+
       MaybeDead->eraseFromParent();
       Changed = true;
       ++NumDeletes;
     }
   }
 
   MaybeDeadCopies.clear();
   Tracker.clear();

llvm/trunk/lib/CodeGen/MachineInstr.cpp

for (MachineBasicBlock::iterator DE = MI.getParent()->end();
        DI != DE; ++DI) {
     if (!DI->isDebugValue())
       return;
     if (DI->getOperand(0).isReg() &&
         DI->getOperand(0).getReg() == MI.getOperand(0).getReg())
       DbgValues.push_back(&*DI);
   }
 }
+
+void MachineInstr::changeDebugValuesDefReg(unsigned Reg) {
+  // Collect matching debug values.
+  SmallVector<MachineInstr *, 2> DbgValues;
+  collectDebugValues(DbgValues);
+
+  // Propagate Reg to debug value instructions.
+  for (auto *DBI : DbgValues)
+    DBI->getOperand(0).setReg(Reg);
+}

llvm/trunk/test/CodeGen/MIR/X86/pr38773.mir

+# RUN: llc -o - %s -mtriple=x86_64-- -run-pass=machine-cp | FileCheck %s
+
+# When MachineCopyPropagation eliminates a dead 'copy', its associated debug
+# information becomes invalid. as the recorded register has been removed.
+# It causes the debugger to display wrong variable value.
+#
+# When in the debugger, on the line "return read1;", the value of "read1"
+# is reported as '4', where it should be '1'.
+#
+# MIR generated with:
+#   clang -S -g -O2 -emit-llvm pr38773.cpp -o pr38773.ll -mllvm
+#   llc pr38773.ll -stop-after=tailduplication -simplify-mir 
+#
+# // pr38773.cpp
+# int main() {
+#   volatile int foo = 4;
+#   int read1 = foo;
+#   int read2 = foo;
+#
+#   switch ((read1 == 4) ? 3 : 1) {
+#   case 1:
+#     read1 *= read2;
+#     break;
+#   case 3:
+#     read1 /= read2;
+#     break;
+#   }
+#    
+#   return read1;
+# }
+#
+# Update the register for the '@llvm.dbg.value' associated with 'read1', when
+# the 'copy' is removed, to be the 'source' register.
+
+--- |
+  ; ModuleID = 'pr38773.ll'
+  source_filename = "pr38773.cpp"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-pc-linux-gnu"
+  
+  define dso_local i32 @main() local_unnamed_addr !dbg !7 {
+  entry:
+    %foo = alloca i32, align 4
+    store volatile i32 4, i32* %foo, align 4
+    %foo.0.foo.0. = load volatile i32, i32* %foo, align 4
+    %foo.0.foo.0.6 = load volatile i32, i32* %foo, align 4
+    %cmp = icmp eq i32 %foo.0.foo.0., 4
+    br i1 %cmp, label %sw.bb1, label %sw.bb
+  
+  sw.bb:                                            ; preds = %entry
+    %mul = mul nsw i32 %foo.0.foo.0.6, %foo.0.foo.0.
+    br label %sw.epilog
+  
+  sw.bb1:                                           ; preds = %entry
+    %div = sdiv i32 4, %foo.0.foo.0.6
+    call void @llvm.dbg.value(metadata i32 %div, metadata !12, metadata !DIExpression()), !dbg !13
+    br label %sw.epilog
+  
+  sw.epilog:                                        ; preds = %sw.bb1, %sw.bb
+    %read1.0 = phi i32 [ %div, %sw.bb1 ], [ %mul, %sw.bb ]
+    call void @llvm.dbg.value(metadata i32 %read1.0, metadata !12, metadata !DIExpression()), !dbg !13
+    ret i32 %read1.0
+  }
+  
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 (trunk 343183)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+  !1 = !DIFile(filename: "pr38773.cpp", 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 8.0.0 (trunk 343183)"}
+  !7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !11)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10}
+  !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !11 = !{!12}
+  !12 = !DILocalVariable(name: "read1", scope: !7, file: !1, line: 3, type: !10)
+  !13 = !DILocation(line: 3, column: 7, scope: !7)
+
+...
+---
+name:            main
+
+body:             |
+  
+  bb.2.sw.bb1:
+    liveins: $ecx
+  
+    $eax = MOV32ri 4
+    $edx = MOV32r0 implicit-def dead $eflags
+    IDIV32r killed renamable $ecx, implicit-def $eax, implicit-def dead $edx, implicit-def dead $eflags, implicit $eax, implicit killed $edx
+    renamable $ecx = COPY $eax
+    ; CHECK:        IDIV32r killed renamable $ecx
+    ; CHECK-NEXT:   DBG_VALUE debug-use $eax, debug-use $noreg, !12, !DIExpression(), debug-location !13
+    DBG_VALUE debug-use $ecx, debug-use $noreg, !12, !DIExpression(), debug-location !13
+    $eax = COPY killed renamable $ecx
+    RET 0, $eax
+
+...