From 9bebc65d7965e12f83ea1ca4d8a0cdb2e079a45b Mon Sep 17 00:00:00 2001 From: Jeremy Morse Date: Tue, 9 Jul 2019 09:38:03 +0000 Subject: [PATCH] Revert r364515 and r364524 Jordan reports on llvm-commits a performance regression with r364515, backing the patch out while it's investigated. llvm-svn: 365448 --- llvm/lib/CodeGen/RegisterCoalescer.cpp | 87 +------- .../MIR/X86/regcoalescing-clears-dead-dbgvals.mir | 243 --------------------- 2 files changed, 2 insertions(+), 328 deletions(-) delete mode 100644 llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp index 535e5bd..2db6ab4 100644 --- a/llvm/lib/CodeGen/RegisterCoalescer.cpp +++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp @@ -265,14 +265,6 @@ 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, @@ -1656,59 +1648,8 @@ void RegisterCoalescer::addUndefFlag(const LiveInterval &Int, SlotIndex UseIdx, } } -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, +void RegisterCoalescer::updateRegDefsUses(unsigned SrcReg, + unsigned DstReg, unsigned SubIdx) { bool DstIsPhys = TargetRegisterInfo::isPhysicalRegister(DstReg); LiveInterval *DstInt = DstIsPhys ? nullptr : &LIS->getInterval(DstReg); @@ -1920,20 +1861,6 @@ 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 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 @@ -2002,16 +1929,6 @@ 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 deleted file mode 100644 index 6a225ab..0000000 --- a/llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir +++ /dev/null @@ -1,243 +0,0 @@ -# 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 - } - - 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 - -... -- 2.7.4