Index: lib/CodeGen/RegisterCoalescer.cpp =================================================================== --- lib/CodeGen/RegisterCoalescer.cpp +++ lib/CodeGen/RegisterCoalescer.cpp @@ -244,6 +244,13 @@ /// Return true if a copy involving a physreg should be joined. bool canJoinPhys(const CoalescerPair &CP); + /// When merging DstLI into the vreg operand of the specified DBG_VALUE, + /// would it change the valuation of the DBG_VALUE? This can happen when + /// the DBG_VALUE is in a non-live range and DstLI is merged into that + /// range. + bool mergingChangesDbgValue(MachineInstr *DbgV, + const LiveInterval &DstLI) 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, @@ -1617,6 +1624,57 @@ } } +bool RegisterCoalescer::mergingChangesDbgValue(MachineInstr *DbgV, + const LiveInterval &DstLI) const { + assert(DbgV->isDebugValue()); + assert(DbgV->getParent() && "DbgValue with no parent"); + const MachineOperand &MO = DbgV->getOperand(0); + if (!MO.isReg()) + return false; + + SlotIndex MIIdx = LIS->getSlotIndexes()->getIndexAfter(*DbgV); + const LiveInterval &SrcLI = LIS->getInterval(MO.getReg()); + + // Now then -- is the src reg live at the point of the DBG_VALUE? + auto LII = SrcLI.find(MIIdx); + if (LII != SrcLI.end() && LII->contains(MIIdx)) + // Reg is live across the DBG_VALUE, so coalescing will protect against + // its valuation changing. + return false; + + // If the src is a use-before-def, it's already unclear which def it was + // supposed to refer to, and it'll be even more unclear after we merge with + // the destination register. Flag this to be undef. This accounts for + // end==begin too. + if (LII == SrcLI.begin()) + return true; + + // Get the value number for whatever was live before this slot. Move to + // previous interval segment, and get live VN at its end. + auto LIIPrev = std::prev(LII); + VNInfo *SrcVN = SrcLI.getVNInfoBefore(LIIPrev->end); + + // If it's not live, and we can't tell what it _was_, it's likely impossible + // to work out what it _should_ be. + if (!SrcVN) + return true; + + // And how about the dest reg? + LII = DstLI.find(MIIdx); + if (LII == DstLI.end() || !LII->contains(MIIdx)) + return false; + + VNInfo *DstVN = DstLI.getVNInfoAt(MIIdx); + + // We know src was dead, dst is live, and thus the DBG_VALUE will be + // resurrected. If src and dst refer to a different def, then there will + // be an unsound alteration. + if (DstVN) + return DstVN->def != SrcVN->def; + else + return false; +} + void RegisterCoalescer::updateRegDefsUses(unsigned SrcReg, unsigned DstReg, unsigned SubIdx) { @@ -1830,6 +1888,21 @@ 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 succeed. + SmallVector DbgValuesToChange; + for (MachineRegisterInfo::reg_instr_iterator + I = MRI->reg_instr_begin(CP.getSrcReg()), + E = MRI->reg_instr_end(); + I != E;) { + MachineInstr *UseMI = &*(I++); + if (UseMI->isDebugValue() && UseMI->getOperand(0).isReg() && + TargetRegisterInfo::isVirtualRegister(CP.getDstReg()) && + mergingChangesDbgValue(UseMI, LIS->getInterval(CP.getDstReg()))) + DbgValuesToChange.push_back(UseMI); + } + // 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 @@ -1898,6 +1971,16 @@ 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()); Index: test/CodeGen/X86/pr40010.mir =================================================================== --- /dev/null +++ test/CodeGen/X86/pr40010.mir @@ -0,0 +1,248 @@ +# 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 + } + + ; 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 +alignment: 4 +exposesReturnsTwice: false +legalized: false +regBankSelected: false +selected: false +failedISel: false +tracksRegLiveness: true +hasWinCFI: false +registers: + - { id: 0, class: gr32, preferred-register: '' } + - { id: 1, class: gr32, preferred-register: '' } + - { id: 2, class: gr64, preferred-register: '' } + - { id: 3, class: gr32, preferred-register: '' } + - { id: 4, class: gr32, preferred-register: '' } + - { id: 5, class: gr32, preferred-register: '' } + - { id: 6, class: gr32, preferred-register: '' } + - { id: 7, class: gr32, preferred-register: '' } + - { id: 8, class: gr32, preferred-register: '' } +liveins: + - { reg: '$rdi', virtual-reg: '%2' } +fixedStack: [] +stack: [] +constants: [] +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: + ; CHECK-LABEL: bb.1.start.test1 + successors: %bb.2(0x04000000), %bb.1(0x7c000000) + + %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 + ; We currently expect %1 and %0 to merge into %7 + %1:gr32 = ADD32rr %1, killed %5, implicit-def dead $eflags + ; CHECK: %7:gr32 = ADD32rr %7 + DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5 + ; CHECK-NEXT: DBG_VALUE $noreg + CMP32ri %1, 1000001, implicit-def $eflags + %7:gr32 = COPY %1 + JB_1 %bb.1, implicit killed $eflags + JMP_1 %bb.2 + + bb.2.leave: + $eax = COPY killed %1 + RET 0, killed $eax + +... +--- +name: test2 +alignment: 4 +exposesReturnsTwice: false +legalized: false +regBankSelected: false +selected: false +failedISel: false +tracksRegLiveness: true +hasWinCFI: false +registers: + - { id: 0, class: gr32, preferred-register: '' } + - { id: 1, class: gr32, preferred-register: '' } + - { id: 2, class: gr64, preferred-register: '' } + - { id: 3, class: gr32, preferred-register: '' } + - { id: 4, class: gr32, preferred-register: '' } + - { id: 5, class: gr32, preferred-register: '' } + - { id: 6, class: gr32, preferred-register: '' } + - { id: 7, class: gr32, preferred-register: '' } + - { id: 8, class: gr32, preferred-register: '' } +liveins: + - { reg: '$rdi', virtual-reg: '%2' } +fixedStack: [] +stack: [] +constants: [] +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: + ; CHECK-LABEL: bb.1.start.test2 + successors: %bb.2(0x04000000), %bb.1(0x7c000000) + + %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 + ; %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. + DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5 + ; CHECK: DBG_VALUE %7, $noreg + %1:gr32 = COPY killed %0 + %1:gr32 = ADD32rr %1, killed %5, implicit-def dead $eflags + ; CHECK: %7:gr32 = ADD32rr %7 + CMP32ri %1, 1000001, implicit-def $eflags + %7:gr32 = COPY %1 + JB_1 %bb.1, implicit killed $eflags + JMP_1 %bb.2 + + bb.2.leave: + $eax = COPY killed %1 + RET 0, killed $eax + +... +--- +name: test3 +alignment: 4 +exposesReturnsTwice: false +legalized: false +regBankSelected: false +selected: false +failedISel: false +tracksRegLiveness: true +hasWinCFI: false +registers: + - { id: 0, class: gr32, preferred-register: '' } + - { id: 1, class: gr32, preferred-register: '' } + - { id: 2, class: gr64, preferred-register: '' } + - { id: 3, class: gr32, preferred-register: '' } + - { id: 4, class: gr32, preferred-register: '' } + - { id: 5, class: gr32, preferred-register: '' } + - { id: 6, class: gr32, preferred-register: '' } + - { id: 7, class: gr32, preferred-register: '' } + - { id: 8, class: gr32, preferred-register: '' } +liveins: + - { reg: '$rdi', virtual-reg: '%2' } +fixedStack: [] +stack: [] +constants: [] +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: + ; CHECK-LABEL: bb.1.start.test3 + successors: %bb.2(0x04000000), %bb.1(0x7c000000) + + ; This is a use-before-def, merging new registers into %0 could unsoundly + ; make it live again, on merge mark it undef. + DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5 + ; CHECK: 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 + ; Check merging happened + ; CHECK: %7:gr32 = ADD32rr %7 + CMP32ri %1, 1000001, implicit-def $eflags + %7:gr32 = COPY %1 + JB_1 %bb.1, implicit killed $eflags + JMP_1 %bb.2 + + bb.2.leave: + $eax = COPY killed %1 + RET 0, killed $eax + +...