[llvm] d9c9a4e - [DebugInfo] Avoid register coalesing unsoundly changing DBG_VALUE locations
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 25 05:52:41 PST 2019
Author: Jeremy Morse
Date: 2019-11-25T13:47:06Z
New Revision: d9c9a4e48d286a04d09a8fdea87f486a9ec02cd0
URL: https://github.com/llvm/llvm-project/commit/d9c9a4e48d286a04d09a8fdea87f486a9ec02cd0
DIFF: https://github.com/llvm/llvm-project/commit/d9c9a4e48d286a04d09a8fdea87f486a9ec02cd0.diff
LOG: [DebugInfo] Avoid register coalesing unsoundly changing DBG_VALUE locations
This is a re-land of D56151 / r364515 with a completely new implementation.
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.
Differential Revision: https://reviews.llvm.org/D64630
Added:
llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir
Modified:
llvm/lib/CodeGen/RegisterCoalescer.cpp
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index ac4bf0ea8d07..59e2e85f29e9 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -120,6 +120,8 @@ static cl::opt<unsigned> LargeIntervalFreqThreshold(
namespace {
+ class JoinVals;
+
class RegisterCoalescer : public MachineFunctionPass,
private LiveRangeEdit::Delegate {
MachineFunction* MF = nullptr;
@@ -131,6 +133,18 @@ namespace {
AliasAnalysis *AA = nullptr;
RegisterClassInfo RegClassInfo;
+ /// Debug variable location tracking -- for each VReg, maintain an
+ /// ordered-by-slot-index set of DBG_VALUEs, to help quick
+ /// identification of whether coalescing may change location validity.
+ using DbgValueLoc = std::pair<SlotIndex, MachineInstr*>;
+ DenseMap<unsigned, std::vector<DbgValueLoc>> DbgVRegToValues;
+
+ /// VRegs may be repeatedly coalesced, and have many DBG_VALUEs attached.
+ /// To avoid repeatedly merging sets of DbgValueLocs, instead record
+ /// which vregs have been coalesced, and where to. This map is from
+ /// vreg => {set of vregs merged in}.
+ DenseMap<unsigned, SmallVector<unsigned, 4>> DbgMergedVRegNums;
+
/// A LaneMask to remember on which subregister live ranges we need to call
/// shrinkToUses() later.
LaneBitmask ShrinkMask;
@@ -327,6 +341,19 @@ namespace {
MI->eraseFromParent();
}
+ /// Walk over function and initialize the DbgVRegToValues map.
+ void buildVRegToDbgValueMap(MachineFunction &MF);
+
+ /// Test whether, after merging, any DBG_VALUEs would refer to a
+ ///
diff erent value number than before merging, and whether this can
+ /// be resolved. If not, mark the DBG_VALUE as being undef.
+ void checkMergingChangesDbgValues(CoalescerPair &CP, LiveRange &LHS,
+ JoinVals &LHSVals, LiveRange &RHS,
+ JoinVals &RHSVals);
+
+ void checkMergingChangesDbgValuesImpl(unsigned Reg, LiveRange &OtherRange,
+ LiveRange &RegRange, JoinVals &Vals2);
+
public:
static char ID; ///< Class identification, replacement for typeinfo
@@ -1650,8 +1677,7 @@ void RegisterCoalescer::addUndefFlag(const LiveInterval &Int, SlotIndex UseIdx,
}
}
-void RegisterCoalescer::updateRegDefsUses(unsigned SrcReg,
- unsigned DstReg,
+void RegisterCoalescer::updateRegDefsUses(unsigned SrcReg, unsigned DstReg,
unsigned SubIdx) {
bool DstIsPhys = Register::isPhysicalRegister(DstReg);
LiveInterval *DstInt = DstIsPhys ? nullptr : &LIS->getInterval(DstReg);
@@ -2197,6 +2223,7 @@ class JoinVals {
/// NewVNInfo. This is suitable for passing to LiveInterval::join().
SmallVector<int, 8> Assignments;
+ public:
/// Conflict resolution for overlapping values.
enum ConflictResolution {
/// No overlap, simply keep this value.
@@ -2225,6 +2252,7 @@ class JoinVals {
CR_Impossible
};
+ private:
/// Per-value info for LI. The lane bit masks are all relative to the final
/// joined register, so they can be compared directly between SrcReg and
/// DstReg.
@@ -2385,6 +2413,11 @@ class JoinVals {
/// Get the value assignments suitable for passing to LiveInterval::join.
const int *getAssignments() const { return Assignments.data(); }
+
+ /// Get the conflict resolution for a value number.
+ ConflictResolution getResolution(unsigned Num) const {
+ return Vals[Num].Resolution;
+ }
};
} // end anonymous namespace
@@ -3387,6 +3420,9 @@ bool RegisterCoalescer::joinVirtRegs(CoalescerPair &CP) {
while (!ShrinkRegs.empty())
shrinkToUses(&LIS->getInterval(ShrinkRegs.pop_back_val()));
+ // Scan and mark undef any DBG_VALUEs that would refer to a
diff erent value.
+ checkMergingChangesDbgValues(CP, LHS, LHSVals, RHS, RHSVals);
+
// Join RHS into LHS.
LHS.join(RHS, LHSVals.getAssignments(), RHSVals.getAssignments(), NewVNInfo);
@@ -3418,6 +3454,140 @@ bool RegisterCoalescer::joinIntervals(CoalescerPair &CP) {
return CP.isPhys() ? joinReservedPhysReg(CP) : joinVirtRegs(CP);
}
+void RegisterCoalescer::buildVRegToDbgValueMap(MachineFunction &MF)
+{
+ const SlotIndexes &Slots = *LIS->getSlotIndexes();
+ SmallVector<MachineInstr *, 8> ToInsert;
+
+ // After collecting a block of DBG_VALUEs into ToInsert, enter them into the
+ // vreg => DbgValueLoc map.
+ auto CloseNewDVRange = [this, &ToInsert](SlotIndex Slot) {
+ for (auto *X : ToInsert)
+ DbgVRegToValues[X->getOperand(0).getReg()].push_back({Slot, X});
+
+ ToInsert.clear();
+ };
+
+ // Iterate over all instructions, collecting them into the ToInsert vector.
+ // Once a non-debug instruction is found, record the slot index of the
+ // collected DBG_VALUEs.
+ for (auto &MBB : MF) {
+ SlotIndex CurrentSlot = Slots.getMBBStartIdx(&MBB);
+
+ for (auto &MI : MBB) {
+ if (MI.isDebugValue() && MI.getOperand(0).isReg() &&
+ MI.getOperand(0).getReg().isVirtual()) {
+ ToInsert.push_back(&MI);
+ } else if (!MI.isDebugInstr()) {
+ CurrentSlot = Slots.getInstructionIndex(MI);
+ CloseNewDVRange(CurrentSlot);
+ }
+ }
+
+ // Close range of DBG_VALUEs at the end of blocks.
+ CloseNewDVRange(Slots.getMBBEndIdx(&MBB));
+ }
+
+ // Sort all DBG_VALUEs we've seen by slot number.
+ for (auto &Pair : DbgVRegToValues)
+ llvm::sort(Pair.second);
+}
+
+void RegisterCoalescer::checkMergingChangesDbgValues(CoalescerPair &CP,
+ LiveRange &LHS,
+ JoinVals &LHSVals,
+ LiveRange &RHS,
+ JoinVals &RHSVals) {
+ auto ScanForDstReg = [&](unsigned Reg) {
+ checkMergingChangesDbgValuesImpl(Reg, RHS, LHS, LHSVals);
+ };
+
+ auto ScanForSrcReg = [&](unsigned Reg) {
+ checkMergingChangesDbgValuesImpl(Reg, LHS, RHS, RHSVals);
+ };
+
+ // Scan for potentially unsound DBG_VALUEs: examine first the register number
+ // Reg, and then any other vregs that may have been merged into it.
+ auto PerformScan = [this](unsigned Reg, std::function<void(unsigned)> Func) {
+ Func(Reg);
+ if (DbgMergedVRegNums.count(Reg))
+ for (unsigned X : DbgMergedVRegNums[Reg])
+ Func(X);
+ };
+
+ // Scan for unsound updates of both the source and destination register.
+ PerformScan(CP.getSrcReg(), ScanForSrcReg);
+ PerformScan(CP.getDstReg(), ScanForDstReg);
+}
+
+void RegisterCoalescer::checkMergingChangesDbgValuesImpl(unsigned Reg,
+ LiveRange &OtherLR,
+ LiveRange &RegLR,
+ JoinVals &RegVals) {
+ // Are there any DBG_VALUEs to examine?
+ auto VRegMapIt = DbgVRegToValues.find(Reg);
+ if (VRegMapIt == DbgVRegToValues.end())
+ return;
+
+ auto &DbgValueSet = VRegMapIt->second;
+ auto DbgValueSetIt = DbgValueSet.begin();
+ auto SegmentIt = OtherLR.begin();
+
+ bool LastUndefResult = false;
+ SlotIndex LastUndefIdx;
+
+ // If the "Other" register is live at a slot Idx, test whether Reg can
+ // safely be merged with it, or should be marked undef.
+ auto ShouldUndef = [&RegVals, &RegLR, &LastUndefResult,
+ &LastUndefIdx](SlotIndex Idx) -> bool {
+ // Our worst-case performance typically happens with asan, causing very
+ // many DBG_VALUEs of the same location. Cache a copy of the most recent
+ // result for this edge-case.
+ if (LastUndefIdx == Idx)
+ return LastUndefResult;
+
+ // If the other range was live, and Reg's was not, the register coalescer
+ // will not have tried to resolve any conflicts. We don't know whether
+ // the DBG_VALUE will refer to the same value number, so it must be made
+ // undef.
+ auto OtherIt = RegLR.find(Idx);
+ if (OtherIt == RegLR.end())
+ return true;
+
+ // Both the registers were live: examine the conflict resolution record for
+ // the value number Reg refers to. CR_Keep meant that this value number
+ // "won" and the merged register definitely refers to that value. CR_Erase
+ // means the value number was a redundant copy of the other value, which
+ // was coalesced and Reg deleted. It's safe to refer to the other register
+ // (which will be the source of the copy).
+ auto Resolution = RegVals.getResolution(OtherIt->valno->id);
+ LastUndefResult = Resolution != JoinVals::CR_Keep &&
+ Resolution != JoinVals::CR_Erase;
+ LastUndefIdx = Idx;
+ return LastUndefResult;
+ };
+
+ // Iterate over both the live-range of the "Other" register, and the set of
+ // DBG_VALUEs for Reg at the same time. Advance whichever one has the lowest
+ // slot index. This relies on the DbgValueSet being ordered.
+ while (DbgValueSetIt != DbgValueSet.end() && SegmentIt != OtherLR.end()) {
+ if (DbgValueSetIt->first < SegmentIt->end) {
+ // "Other" is live and there is a DBG_VALUE of Reg: test if we should
+ // set it undef.
+ if (DbgValueSetIt->first >= SegmentIt->start &&
+ DbgValueSetIt->second->getOperand(0).getReg() != 0 &&
+ ShouldUndef(DbgValueSetIt->first)) {
+ // Mark undef, erase record of this DBG_VALUE to avoid revisiting.
+ DbgValueSetIt->second->getOperand(0).setReg(0);
+ continue;
+ }
+ ++DbgValueSetIt;
+ } else {
+ ++SegmentIt;
+ }
+ }
+}
+
namespace {
/// Information concerning MBB coalescing priority.
@@ -3700,6 +3870,10 @@ bool RegisterCoalescer::runOnMachineFunction(MachineFunction &fn) {
if (VerifyCoalescing)
MF->verify(this, "Before register coalescing");
+ DbgVRegToValues.clear();
+ DbgMergedVRegNums.clear();
+ buildVRegToDbgValueMap(fn);
+
RegClassInfo.runOnMachineFunction(fn);
// Join (coalesce) intervals if requested.
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 000000000000..feadfb26f41e
--- /dev/null
+++ b/llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir
@@ -0,0 +1,145 @@
+# RUN: llc -mtriple=x86_64-unknown-unknown %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
+ }
+
+ ; 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.
+ ; RegisterCoalescer resolves %0 as CR_Erase: %0 is a redundant copy and
+ ; can be erased.
+ ;
+ ; 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)
+ %8:gr32 = XOR32rr %8, %4, implicit-def dead $eflags
+ DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5
+ %0:gr32 = ADD32rr %0, killed %8, implicit-def dead $eflags
+ CMP32ri %0, 1000001, implicit-def $eflags
+ %7:gr32 = COPY %0
+ JCC_1 %bb.1, 2, implicit killed $eflags
+ JMP_1 %bb.2
+
+ bb.2.leave:
+ $eax = COPY killed %7
+ RET 0, killed $eax
+
+...
+
More information about the llvm-commits
mailing list