[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 18:09:16 PDT 2019
Forgot to mention, we don't have specific timings yet, but we're seeing
files take >15min to compile that were previously taking a minute or two --
so it's a pretty large increase.
On Mon, Jul 8, 2019 at 4:24 PM Jordan Rupprecht <rupprecht at google.com>
wrote:
> 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/2ba48890/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/2ba48890/attachment.bin>
More information about the llvm-commits
mailing list