From: Jeremy Morse <jeremy.morse.llvm@gmail.com>
Date: Thu, 27 Jun 2019 10:20:27 +0000 (+0000)
Subject: [DebugInfo] Avoid register coalesing unsoundly changing DBG_VALUE locations
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d528bcd9657312f6317d304fa486f42b034b735c;p=platform%2Fupstream%2Fllvm.git

[DebugInfo] Avoid register coalesing unsoundly changing DBG_VALUE locations

Once MIR code leaves SSA form and the liveness of a vreg is considered,
DBG_VALUE insts are able to refer to non-live vregs, because their
debug-uses do not contribute to liveness. This non-liveness becomes
problematic for optimizations like register coalescing, as they can't
``see'' the debug uses in the liveness analyses.

As a result registers get coalesced regardless of debug uses, and that can
lead to invalid variable locations containing unexpected values. In the
added test case, the first vreg operand of ADD32rr is merged with various
copies of the vreg (great for performance), but a DBG_VALUE of the
unmodified operand is blindly updated to the modified operand. This changes
what value the variable will appear to have in a debugger.

Fix this by changing any DBG_VALUE whose operand will be resurrected by
register coalescing to be a $noreg DBG_VALUE, i.e. give the variable no
location. This is an overapproximation as some coalesced locations are
safe (others are not) -- an extra domination analysis would be required to
work out which, and it would be better if we just don't generate non-live
DBG_VALUEs.

This fixes PR40010.

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

llvm-svn: 364515
---

diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index f747b5d6..0dc5ae1 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -265,6 +265,14 @@ namespace {
     /// Return true if a copy involving a physreg should be joined.
     bool canJoinPhys(const CoalescerPair &CP);
 
+    /// When merging SrcReg and DstReg together, and the operand of the
+    /// specified DBG_VALUE refers to one of them, would the def that a
+    /// DBG_VALUE refers to change? This can happen when the DBG_VALUEs
+    /// operand is dead and it's merged into a different live value,
+    /// meaning the DBG_VALUE operands must be updated.
+    bool mergingChangesDbgValue(MachineInstr *DbgV, unsigned SrcReg,
+                                unsigned DstReg) const;
+
     /// Replace all defs and uses of SrcReg to DstReg and update the subregister
     /// number if it is not zero. If DstReg is a physical register and the
     /// existing subregister number of the def / use being updated is not zero,
@@ -1641,8 +1649,59 @@ void RegisterCoalescer::addUndefFlag(const LiveInterval &Int, SlotIndex UseIdx,
   }
 }
 
-void RegisterCoalescer::updateRegDefsUses(unsigned SrcReg,
-                                          unsigned DstReg,
+bool RegisterCoalescer::mergingChangesDbgValue(MachineInstr *DbgV,
+                                               unsigned SrcReg,
+                                               unsigned DstReg) const {
+  assert(DbgV->isDebugValue());
+  assert(DbgV->getParent() && "DbgValue with no parent");
+  assert(DbgV->getOperand(0).isReg());
+  unsigned DbgReg = DbgV->getOperand(0).getReg();
+
+  SlotIndex MIIdx = LIS->getSlotIndexes()->getIndexAfter(*DbgV);
+  const LiveInterval &SrcLI = LIS->getInterval(SrcReg);
+
+  // Is the source register live across the DBG_VALUE?
+  bool SrcLive = false;
+  auto LII = SrcLI.find(MIIdx);
+  if (LII != SrcLI.end() && LII->contains(MIIdx))
+    SrcLive = true;
+
+  bool DstLive = false;
+  // Destination register can be physical or virtual.
+  if (TargetRegisterInfo::isVirtualRegister(DstReg)) {
+    // Is DstReg live across the DBG_VALUE?
+    const LiveInterval &DstLI = LIS->getInterval(DstReg);
+    LII = DstLI.find(MIIdx);
+    DstLive = (LII != DstLI.end() && LII->contains(MIIdx));
+  } else if (MRI->isConstantPhysReg(DstReg)) {
+    // Constant physical registers are always live.
+    DstLive = true;
+  } else {
+    // For physical registers, see if any register unit containing DstReg
+    // is live across the DBG_VALUE.
+    for (MCRegUnitIterator UI(DstReg, TRI); UI.isValid(); ++UI) {
+      const LiveRange &DstLI = LIS->getRegUnit(*UI);
+      auto DstLII = DstLI.find(MIIdx);
+      if (DstLII != DstLI.end() && DstLII->contains(MIIdx)) {
+        DstLive = true;
+        break;
+      }
+    }
+  }
+
+  // We now know whether src and dst are live. Best case: we have a DBG_VALUE
+  // of a live register, coalesing won't change its value.
+  if ((DstLive && DbgReg == DstReg) || (SrcLive && DbgReg == SrcReg))
+    return false;
+  // If neither register are live, no damage done.
+  if (!DstLive && !SrcLive)
+    return false;
+  // Otherwise, we will end up resurrecting the DBG_VALUE with a different
+  // register, which is unsafe.
+  return true;
+}
+
+void RegisterCoalescer::updateRegDefsUses(unsigned SrcReg, unsigned DstReg,
                                           unsigned SubIdx) {
   bool DstIsPhys = TargetRegisterInfo::isPhysicalRegister(DstReg);
   LiveInterval *DstInt = DstIsPhys ? nullptr : &LIS->getInterval(DstReg);
@@ -1854,6 +1913,20 @@ bool RegisterCoalescer::joinCopy(MachineInstr *CopyMI, bool &Again) {
   ShrinkMask = LaneBitmask::getNone();
   ShrinkMainRange = false;
 
+  // Although we can update the DBG_VALUEs to the merged register, as debug uses
+  // do not contribute to liveness it might not be a sound update. Collect
+  // DBG_VALUEs that would change value were this interval merging to succeed.
+  SmallVector<MachineInstr *, 4> DbgValuesToChange;
+  auto CheckForDbgUser = [this, &CP, &DbgValuesToChange](MachineInstr &MI) {
+    if (MI.isDebugValue() && MI.getOperand(0).isReg() &&
+        mergingChangesDbgValue(&MI, CP.getSrcReg(), CP.getDstReg()))
+      DbgValuesToChange.push_back(&MI);
+  };
+  for (auto &RegIt : MRI->reg_instructions(CP.getSrcReg()))
+    CheckForDbgUser(RegIt);
+  for (auto &RegIt : MRI->reg_instructions(CP.getDstReg()))
+    CheckForDbgUser(RegIt);
+
   // Okay, attempt to join these two intervals.  On failure, this returns false.
   // Otherwise, if one of the intervals being joined is a physreg, this method
   // always canonicalizes DstInt to be it.  The output "SrcInt" will not have
@@ -1922,6 +1995,16 @@ bool RegisterCoalescer::joinCopy(MachineInstr *CopyMI, bool &Again) {
     updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx());
   updateRegDefsUses(CP.getSrcReg(), CP.getDstReg(), CP.getSrcIdx());
 
+  // The updates to these DBG_VALUEs are not sound -- mark them undef.
+  // FIXME: further analysis might recover them, this is the minimal sound
+  // solution.
+  for (MachineInstr *MI : DbgValuesToChange) {
+    assert(MI->getOperand(0).isReg());
+    LLVM_DEBUG(dbgs() << "Update of " << MI->getOperand(0) << " would be "
+                      << "unsound, setting undef\n");
+    MI->getOperand(0).setReg(0);
+  }
+
   // Shrink subregister ranges if necessary.
   if (ShrinkMask.any()) {
     LiveInterval &LI = LIS->getInterval(CP.getDstReg());
diff --git a/llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir b/llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir
new file mode 100644
index 0000000..a5d707e
--- /dev/null
+++ b/llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir
@@ -0,0 +1,243 @@
+# RUN: llc %s -o - -run-pass=simple-register-coalescing | FileCheck %s
+# PR40010: DBG_VALUEs do not contribute to the liveness of virtual registers,
+# and the register coalescer would merge new live values on top of DBG_VALUEs,
+# leading to them presenting new (wrong) values to the debugger. Test that
+# when out of liveness, coalescing will mark DBG_VALUEs in non-live locations
+# as undef.
+--- |
+  ; ModuleID = './test.ll'
+  source_filename = "./test.ll"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+  ; Function Attrs: nounwind readnone speculatable
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+  ; Original IR source here:
+  define i32 @test(i32* %pin) {
+  entry:
+    br label %start.test1
+
+  start.test1:                                       ; preds = %start, %entry
+    %foo = phi i32 [ 0, %entry ], [ %bar, %start.test1 ]
+    %baz = load i32, i32* %pin, align 1
+    %qux = xor i32 %baz, 1234
+    %bar = add i32 %qux, %foo
+    call void @llvm.dbg.value(metadata i32 %foo, metadata !3, metadata !DIExpression()), !dbg !5
+    %cmp = icmp ugt i32 %bar, 1000000
+    br i1 %cmp, label %leave, label %start.test1
+
+  leave:                                            ; preds = %start
+    ret i32 %bar
+  }
+
+  ; Stubs to appease the MIR parser
+  define i32 @test2(i32* %pin) {
+  entry:
+    ret i32 0
+  start.test2:
+    ret i32 0
+  leave:
+    ret i32 0
+  }
+
+  define i32 @test3(i32* %pin) {
+  entry:
+    ret i32 0
+  start.test3:
+    ret i32 0
+  leave:
+    ret i32 0
+  }
+
+  define i32 @test4(i32* %pin) {
+  entry:
+    ret i32 0
+  start.test4:
+    ret i32 0
+  leave:
+    ret i32 0
+  }
+
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**) #1
+
+  attributes #0 = { nounwind readnone speculatable }
+  attributes #1 = { nounwind }
+
+  !llvm.module.flags = !{!0}
+  !llvm.dbg.cu = !{!1}
+
+  !0 = !{i32 2, !"Debug Info Version", i32 3}
+  !1 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !2, producer: "beards", isOptimized: true, runtimeVersion: 4, emissionKind: FullDebug)
+  !2 = !DIFile(filename: "bees.cpp", directory: "")
+  !3 = !DILocalVariable(name: "bees", scope: !4)
+  !4 = distinct !DISubprogram(name: "nope", scope: !1, file: !2, line: 1, spFlags: DISPFlagDefinition, unit: !1)
+  !5 = !DILocation(line: 0, scope: !4)
+
+...
+---
+name:            test
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x80000000)
+    liveins: $rdi
+
+    %2:gr64 = COPY killed $rdi
+    %3:gr32 = MOV32r0 implicit-def dead $eflags
+    %4:gr32 = MOV32ri 1234
+    %7:gr32 = COPY killed %3
+
+  bb.1.start.test1:
+    successors: %bb.2(0x04000000), %bb.1(0x7c000000)
+
+    ; CHECK-LABEL: name: test
+    ;
+    ; We currently expect %1 and %0 to merge into %7
+    ;
+    ; CHECK: %[[REG1:[0-9]+]]:gr32 = MOV32rm
+    ; CHECK-NEXT: %[[REG2:[0-9]+]]:gr32 = XOR32rr %[[REG1]]
+    ; CHECK-NEXT: %[[REG3:[0-9]+]]:gr32 = ADD32rr %[[REG3]], %[[REG2]]
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    %0:gr32 = COPY killed %7
+    %8:gr32 = MOV32rm %2, 1, $noreg, 0, $noreg :: (load 4 from %ir.pin, align 1)
+    %5:gr32 = COPY killed %8
+    %5:gr32 = XOR32rr %5, %4, implicit-def dead $eflags
+    %1:gr32 = COPY killed %0
+    %1:gr32 = ADD32rr %1, killed %5, implicit-def dead $eflags
+    DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5
+    CMP32ri %1, 1000001, implicit-def $eflags
+    %7:gr32 = COPY %1
+    JCC_1 %bb.1, 2, implicit killed $eflags
+    JMP_1 %bb.2
+
+  bb.2.leave:
+    $eax = COPY killed %1
+    RET 0, killed $eax
+
+...
+---
+name:            test2
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x80000000)
+    liveins: $rdi
+
+    %2:gr64 = COPY killed $rdi
+    %3:gr32 = MOV32r0 implicit-def dead $eflags
+    %4:gr32 = MOV32ri 1234
+    %7:gr32 = COPY killed %3
+
+  bb.1.start.test2:
+    successors: %bb.2(0x04000000), %bb.1(0x7c000000)
+
+    ; CHECK-LABEL: name: test2
+    ;
+    ; %0 should be merged into %7, but as %0 is live at this location the
+    ; DBG_VALUE should be preserved and point at the operand of ADD32rr.
+    ;
+    ; CHECK: %[[REG11:[0-9]+]]:gr32 = MOV32rm
+    ; CHECK-NEXT: %[[REG12:[0-9]+]]:gr32 = XOR32rr %[[REG11]]
+    ; CHECK-NEXT: DBG_VALUE %[[REG13:[0-9]+]]
+    ; CHECK-NEXT: %[[REG13]]:gr32 = ADD32rr %[[REG13]], %[[REG12]]
+
+    %0:gr32 = COPY killed %7
+    %8:gr32 = MOV32rm %2, 1, $noreg, 0, $noreg :: (load 4 from %ir.pin, align 1)
+    %5:gr32 = COPY killed %8
+    %5:gr32 = XOR32rr %5, %4, implicit-def dead $eflags
+    DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5
+    %1:gr32 = COPY killed %0
+    %1:gr32 = ADD32rr %1, killed %5, implicit-def dead $eflags
+    CMP32ri %1, 1000001, implicit-def $eflags
+    %7:gr32 = COPY %1
+    JCC_1 %bb.1, 2, implicit killed $eflags
+    JMP_1 %bb.2
+
+  bb.2.leave:
+    $eax = COPY killed %1
+    RET 0, killed $eax
+
+...
+---
+name:            test3
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x80000000)
+    liveins: $rdi
+
+    %2:gr64 = COPY killed $rdi
+    %3:gr32 = MOV32r0 implicit-def dead $eflags
+    %4:gr32 = MOV32ri 1234
+    %7:gr32 = COPY killed %3
+
+  bb.1.start.test3:
+    successors: %bb.2(0x04000000), %bb.1(0x7c000000)
+
+    ; CHECK-LABEL: name: test3
+    ;
+    ; This is a use-before-def, merging new registers into %0 could unsoundly
+    ; make it live again, on merge mark it undef.
+    ;
+    ; CHECK: DBG_VALUE $noreg
+
+    DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5
+    %0:gr32 = COPY killed %7
+    %8:gr32 = MOV32rm %2, 1, $noreg, 0, $noreg :: (load 4 from %ir.pin, align 1)
+    %5:gr32 = COPY killed %8
+    %5:gr32 = XOR32rr %5, %4, implicit-def dead $eflags
+    %1:gr32 = COPY killed %0
+    %1:gr32 = ADD32rr %1, killed %5, implicit-def dead $eflags
+    CMP32ri %1, 1000001, implicit-def $eflags
+    %7:gr32 = COPY %1
+    JCC_1 %bb.1, 2, implicit killed $eflags
+    JMP_1 %bb.2
+
+  bb.2.leave:
+    $eax = COPY killed %1
+    RET 0, killed $eax
+
+...
+---
+name:            test4
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x80000000)
+    liveins: $rdi
+
+    %2:gr64 = COPY killed $rdi
+    %3:gr32 = MOV32r0 implicit-def dead $eflags
+    %4:gr32 = MOV32ri 1234
+    %7:gr32 = COPY killed %3
+
+  bb.1.start.test4:
+    successors: %bb.2(0x04000000), %bb.1(0x7c000000)
+
+    ; CHECK-LABEL: name: test4
+    ;
+    ; Using a dead register, even if we coalesce it to the right value, should
+    ; be marked undef. The coalescer can't prove it's correct without
+    ; considering control flow in the general case.
+    ;
+    ; CHECK: DBG_VALUE $noreg
+
+    %0:gr32 = COPY killed %7
+    DBG_VALUE %7, $noreg, !3, !DIExpression(), debug-location !5
+    %8:gr32 = MOV32rm %2, 1, $noreg, 0, $noreg :: (load 4 from %ir.pin, align 1)
+    %5:gr32 = COPY killed %8
+    %5:gr32 = XOR32rr %5, %4, implicit-def dead $eflags
+    %1:gr32 = COPY killed %0
+    %1:gr32 = ADD32rr %1, killed %5, implicit-def dead $eflags
+    CMP32ri %1, 1000001, implicit-def $eflags
+    %7:gr32 = COPY %1
+    JCC_1 %bb.1, 2, implicit killed $eflags
+    JMP_1 %bb.2
+
+  bb.2.leave:
+    $eax = COPY killed %1
+    RET 0, killed $eax
+
+...