[llvm] r364515 - [DebugInfo] Avoid register coalesing unsoundly changing DBG_VALUE locations
Jordan Rupprecht via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 8 16:24:59 PDT 2019
Hi Jeremy,
We're seeing some compilation timeouts as a result of this patch. Mind if
we revert this until we can get a public reproducer w/ timings showing the
regression?
On Thu, Jun 27, 2019 at 3:20 AM Jeremy Morse via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: jmorse
> Date: Thu Jun 27 03:20:27 2019
> New Revision: 364515
>
> URL: http://llvm.org/viewvc/llvm-project?rev=364515&view=rev
> Log:
> [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
>
> Added:
> llvm/trunk/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir
> Modified:
> llvm/trunk/lib/CodeGen/RegisterCoalescer.cpp
>
> Modified: llvm/trunk/lib/CodeGen/RegisterCoalescer.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegisterCoalescer.cpp?rev=364515&r1=364514&r2=364515&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/RegisterCoalescer.cpp (original)
> +++ llvm/trunk/lib/CodeGen/RegisterCoalescer.cpp Thu Jun 27 03:20:27 2019
> @@ -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(con
> }
> }
>
> -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(Machine
> 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(Machine
> 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());
>
> Added:
> llvm/trunk/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir?rev=364515&view=auto
>
> ==============================================================================
> ---
> llvm/trunk/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir
> (added)
> +++
> llvm/trunk/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir Thu
> Jun 27 03:20:27 2019
> @@ -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
> +
> +...
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190708/c2aae894/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4849 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190708/c2aae894/attachment.bin>
More information about the llvm-commits
mailing list