diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp --- a/llvm/lib/CodeGen/MachineSink.cpp +++ b/llvm/lib/CodeGen/MachineSink.cpp @@ -15,6 +15,8 @@ // //===----------------------------------------------------------------------===// +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/PointerIntPair.h" #include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallVector.h" @@ -106,8 +108,24 @@ using AllSuccsCache = std::map>; - // Remember debug uses of vregs seen, so we know what to sink out of blocks. - DenseMap> SeenDbgUsers; + /// DBG_VALUE pointer and flag. The flag is true if this DBG_VALUE is + /// post-dominated by another DBG_VALUE of the same variable location. + /// This is necessary to detect sequences such as: + /// %0 = someinst + /// DBG_VALUE %0, !123, !DIExpression() + /// %1 = anotherinst + /// DBG_VALUE %1, !123, !DIExpression() + /// Where if %0 were to sink, the DBG_VAUE should not sink with it, as that + /// would re-order assignments. + using SeenDbgUser = PointerIntPair; + + /// Record of DBG_VALUE uses of vregs in a block, so that we can identify + /// debug instructions to sink. + SmallDenseMap> SeenDbgUsers; + + /// Record of debug variables that have had their locations set in the + /// current block. + DenseSet SeenDbgVars; public: static char ID; // Pass identification @@ -399,6 +417,7 @@ } while (!ProcessedBegin); SeenDbgUsers.clear(); + SeenDbgVars.clear(); return MadeChange; } @@ -408,11 +427,16 @@ // we know what to sink if the vreg def sinks. assert(MI.isDebugValue() && "Expected DBG_VALUE for processing"); + DebugVariable Var(MI.getDebugVariable(), MI.getDebugExpression(), + MI.getDebugLoc()->getInlinedAt()); + bool SeenBefore = SeenDbgVars.count(Var) != 0; + MachineOperand &MO = MI.getOperand(0); - if (!MO.isReg() || !MO.getReg().isVirtual()) - return; + if (MO.isReg() && MO.getReg().isVirtual()) + SeenDbgUsers[MO.getReg()].push_back(SeenDbgUser(&MI, SeenBefore)); - SeenDbgUsers[MO.getReg()].push_back(&MI); + // Record the variable for any DBG_VALUE, to avoid re-ordering any of them. + SeenDbgVars.insert(Var); } bool MachineSinking::isWorthBreakingCriticalEdge(MachineInstr &MI, @@ -759,12 +783,60 @@ MBP.LHS.getReg() == BaseOp->getReg(); } +/// If the sunk instruction is a copy, try to forward the copy instead of +/// leaving an 'undef' DBG_VALUE in the original location. Don't do this if +/// there's any subregister weirdness involved. Returns true if copy +/// propagation occurred. +static bool attemptDebugCopyProp(MachineInstr &SinkInst, MachineInstr &DbgMI) { + const MachineRegisterInfo &MRI = SinkInst.getMF()->getRegInfo(); + const TargetInstrInfo &TII = *SinkInst.getMF()->getSubtarget().getInstrInfo(); + + // Copy DBG_VALUE operand and set the original to undef. We then check to + // see whether this is something that can be copy-forwarded. If it isn't, + // continue around the loop. + MachineOperand DbgMO = DbgMI.getOperand(0); + + const MachineOperand *SrcMO = nullptr, *DstMO = nullptr; + auto CopyOperands = TII.isCopyInstr(SinkInst); + if (!CopyOperands) + return false; + SrcMO = CopyOperands->Source; + DstMO = CopyOperands->Destination; + + // Check validity of forwarding this copy. + bool PostRA = MRI.getNumVirtRegs() == 0; + + // Trying to forward between physical and virtual registers is too hard. + if (DbgMO.getReg().isVirtual() != SrcMO->getReg().isVirtual()) + return false; + + // Only try virtual register copy-forwarding before regalloc, and physical + // register copy-forwarding after regalloc. + bool arePhysRegs = !DbgMO.getReg().isVirtual(); + if (arePhysRegs != PostRA) + return false; + + // Pre-regalloc, only forward if all subregisters agree (or there are no + // subregs at all). More analysis might recover some forwardable copies. + if (!PostRA && (DbgMO.getSubReg() != SrcMO->getSubReg() || + DbgMO.getSubReg() != DstMO->getSubReg())) + return false; + + // Post-regalloc, we may be sinking a DBG_VALUE of a sub or super-register + // of this copy. Only forward the copy if the DBG_VALUE operand exactly + // matches the copy destination. + if (PostRA && DbgMO.getReg() != DstMO->getReg()) + return false; + + DbgMI.getOperand(0).setReg(SrcMO->getReg()); + DbgMI.getOperand(0).setSubReg(SrcMO->getSubReg()); + return true; +} + /// Sink an instruction and its associated debug instructions. static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo, MachineBasicBlock::iterator InsertPos, SmallVectorImpl &DbgValuesToSink) { - const MachineRegisterInfo &MRI = MI.getMF()->getRegInfo(); - const TargetInstrInfo &TII = *MI.getMF()->getSubtarget().getInstrInfo(); // If we cannot find a location to use (merge with), then we erase the debug // location to prevent debug-info driven tools from potentially reporting @@ -784,9 +856,6 @@ // DBG_VALUE location as 'undef', indicating that any earlier variable // location should be terminated as we've optimised away the value at this // point. - // If the sunk instruction is a copy, try to forward the copy instead of - // leaving an 'undef' DBG_VALUE in the original location. Don't do this if - // there's any subregister weirdness involved. for (SmallVectorImpl::iterator DBI = DbgValuesToSink.begin(), DBE = DbgValuesToSink.end(); DBI != DBE; ++DBI) { @@ -794,46 +863,8 @@ MachineInstr *NewDbgMI = DbgMI->getMF()->CloneMachineInstr(*DBI); SuccToSinkTo.insert(InsertPos, NewDbgMI); - // Copy DBG_VALUE operand and set the original to undef. We then check to - // see whether this is something that can be copy-forwarded. If it isn't, - // continue around the loop. - MachineOperand DbgMO = DbgMI->getOperand(0); - DbgMI->getOperand(0).setReg(0); - - const MachineOperand *SrcMO = nullptr, *DstMO = nullptr; - auto CopyOperands = TII.isCopyInstr(MI); - if (!CopyOperands) - continue; - SrcMO = CopyOperands->Source; - DstMO = CopyOperands->Destination; - - // Check validity of forwarding this copy. - bool PostRA = MRI.getNumVirtRegs() == 0; - - // Trying to forward between physical and virtual registers is too hard. - if (DbgMO.getReg().isVirtual() != SrcMO->getReg().isVirtual()) - continue; - - // Only try virtual register copy-forwarding before regalloc, and physical - // register copy-forwarding after regalloc. - bool arePhysRegs = !DbgMO.getReg().isVirtual(); - if (arePhysRegs != PostRA) - continue; - - // Pre-regalloc, only forward if all subregisters agree (or there are no - // subregs at all). More analysis might recover some forwardable copies. - if (!PostRA && (DbgMO.getSubReg() != SrcMO->getSubReg() || - DbgMO.getSubReg() != DstMO->getSubReg())) - continue; - - // Post-regalloc, we may be sinking a DBG_VALUE of a sub or super-register - // of this copy. Only forward the copy if the DBG_VALUE operand exactly - // matches the copy destination. - if (PostRA && DbgMO.getReg() != DstMO->getReg()) - continue; - - DbgMI->getOperand(0).setReg(SrcMO->getReg()); - DbgMI->getOperand(0).setSubReg(SrcMO->getSubReg()); + if (!attemptDebugCopyProp(MI, *DbgMI)) + DbgMI->getOperand(0).setReg(0); } } @@ -959,8 +990,19 @@ if (!SeenDbgUsers.count(MO.getReg())) continue; + // Sink any users that don't pass any other DBG_VALUEs for this variable. auto &Users = SeenDbgUsers[MO.getReg()]; - DbgUsersToSink.insert(DbgUsersToSink.end(), Users.begin(), Users.end()); + for (auto &User : Users) { + MachineInstr *DbgMI = User.getPointer(); + if (User.getInt()) { + // This DBG_VALUE would re-order assignments. If we can't copy-propagate + // it, it can't be recovered. Set it undef. + if (!attemptDebugCopyProp(MI, *DbgMI)) + DbgMI->getOperand(0).setReg(0); + } else { + DbgUsersToSink.push_back(DbgMI); + } + } } // After sinking, some debug users may not be dominated any more. If possible, diff --git a/llvm/test/DebugInfo/MIR/X86/machinesink.mir b/llvm/test/DebugInfo/MIR/X86/machinesink.mir --- a/llvm/test/DebugInfo/MIR/X86/machinesink.mir +++ b/llvm/test/DebugInfo/MIR/X86/machinesink.mir @@ -1,10 +1,14 @@ # RUN: llc -mtriple=x86_64-unknown-unknown -run-pass=machine-sink -o - %s | FileCheck %s -# Check various things when we sink machine instructions: +# In the first test, check various things when we sink machine instructions: # a) DBG_VALUEs should sink with defs # b) Undefs should be left behind # c) DBG_VALUEs not immediately following the defining inst should sink too # d) If we generate debug-use-before-defs through sinking, and can copy # propagate to a different value, we should do that. +# +# Test two checks DBG_VALUEs do not sink past a DBG_VALUE in the same block +# that redefines the variable location, while three checks that copy-propagation +# happens in the same scenario. --- | target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" @@ -22,6 +26,26 @@ ret void } + define void @test2(i32* nocapture readonly %p) local_unnamed_addr !dbg !101 { + ; Stripped + entry: + br label %block1 + block1: + br label %exit + exit: + ret void + } + + define void @test3(i32* nocapture readonly %p) local_unnamed_addr !dbg !201 { + ; Stripped + entry: + br label %block1 + block1: + br label %exit + exit: + ret void + } + ; Function Attrs: nounwind readnone declare void @llvm.dbg.value(metadata, i64, metadata, metadata) @@ -49,8 +73,18 @@ !17 = !DIExpression() !18 = !DILocation(line: 2, column: 34, scope: !9) !28 = !DILexicalBlockFile(scope: !9, file: !2, discriminator: 1) + !101 = distinct !DISubprogram(name: "test2", scope: !2, file: !2, line: 2, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !1, retainedNodes: !102) + !102 = !{!103} + !103 = !DILocalVariable(name: "q", arg: 1, scope: !101, file: !2, line: 2, type: !12) + !104 = !DILocation(line: 1, column: 1, scope: !101) + !201 = distinct !DISubprogram(name: "test2", scope: !2, file: !2, line: 2, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !1, retainedNodes: !202) + !202 = !{!203} + !203 = !DILocalVariable(name: "r", arg: 1, scope: !201, file: !2, line: 2, type: !12) + !204 = !DILocation(line: 1, column: 1, scope: !201) ; CHECK: [[VARNUM:![0-9]+]] = !DILocalVariable(name: "p", + ; CHECK: [[VAR2NUM:![0-9]+]] = !DILocalVariable(name: "q", + ; CHECK: [[VAR3NUM:![0-9]+]] = !DILocalVariable(name: "r", ... --- @@ -103,3 +137,101 @@ $rax = MOV64rr %2 RET 0 ... +--- +name: test2 +tracksRegLiveness: true +liveins: + - { reg: '$rdi', virtual-reg: '%2' } + - { reg: '$rsi', virtual-reg: '%2' } +body: | + bb.0.entry: + successors: %bb.1.block1, %bb.2.exit + liveins: $rdi, $esi + + ; This block should _not_ have the first DBG_VALUE sunk out from it, as it + ; would pass a later DBG_VALUE of the same variable location. An undef + ; DBG_VALUE should be left behind though. + ; CHECK-LABEL: bb.0.entry: + ; CHECK: [[TEST2VREG:%[0-9]+]]:gr64 = COPY $rdi + ; CHECK-NEXT: CMP32ri $esi, 0 + ; CHECK-NEXT: DBG_VALUE $noreg, $noreg, [[VAR2NUM]] + ; CHECK-NEXT: CMP32ri $esi, 0 + ; CHECK-NEXT: DBG_VALUE 0, $noreg, [[VAR2NUM]] + ; CHECK-NEXT: JCC_1 %bb.1, 4 + ; CHECK-NEXT: JMP_1 + + %2:gr64 = COPY $rdi + %5:gr64 = ADD64ri8 %2, 1, implicit-def dead $eflags + CMP32ri $esi, 0, implicit-def $eflags + DBG_VALUE %5, $noreg, !103, !17, debug-location !104 + CMP32ri $esi, 0, implicit-def $eflags + DBG_VALUE 0, $noreg, !103, !17, debug-location !104 + JCC_1 %bb.1.block1, 4, implicit $eflags + JMP_1 %bb.2.exit + + bb.1.block1: + successors: %bb.2.exit + + ; This block should receive no DBG_VALUE. + ; CHECK-LABEL: bb.1.block1: + ; CHECK-NOT: DBG_VALUE + ; CHECK: [[SUNKVREG2:%[0-9]+]]:gr64 = ADD64ri8 [[TEST2VREG]] + ; CHECK-NOT: DBG_VALUE + ; CHECK-NEXT: ADD64ri8 + ; CHECK: JMP_1 %bb.2 + %1:gr64 = ADD64ri8 %5, 4, implicit-def dead $eflags + JMP_1 %bb.2.exit + + bb.2.exit: + $rax = MOV64rr %2 + RET 0 +... +--- +name: test3 +tracksRegLiveness: true +liveins: + - { reg: '$rdi', virtual-reg: '%2' } + - { reg: '$rsi', virtual-reg: '%2' } +body: | + bb.0.entry: + successors: %bb.1.block1, %bb.2.exit + liveins: $rdi, $esi + + ; The copy from %2 to %5 will sink into a later block, and the first + ; DBG_VALUE cannot pass the second. But, we should be able to recover + ; the variable location through copy propagation. + ; CHECK-LABEL: bb.0.entry: + ; CHECK: [[TEST3VREG:%[0-9]+]]:gr64 = COPY $rdi + ; CHECK-NEXT: CMP32ri $esi, 0 + ; CHECK-NEXT: DBG_VALUE [[TEST3VREG]], $noreg, [[VAR3NUM]] + ; CHECK-NEXT: CMP32ri $esi, 0 + ; CHECK-NEXT: DBG_VALUE 0, $noreg, [[VAR3NUM]] + ; CHECK-NEXT: JCC_1 %bb.1, 4 + ; CHECK-NEXT: JMP_1 + + %2:gr64 = COPY $rdi + %5:gr64 = COPY %2 + CMP32ri $esi, 0, implicit-def $eflags + DBG_VALUE %5, $noreg, !203, !17, debug-location !204 + CMP32ri $esi, 0, implicit-def $eflags + DBG_VALUE 0, $noreg, !203, !17, debug-location !204 + JCC_1 %bb.1.block1, 4, implicit $eflags + JMP_1 %bb.2.exit + + bb.1.block1: + successors: %bb.2.exit + + ; This block should receive no DBG_VALUE. + ; CHECK-LABEL: bb.1.block1: + ; CHECK-NOT: DBG_VALUE + ; CHECK: COPY [[TEST3VREG]] + ; CHECK-NOT: DBG_VALUE + ; CHECK-NEXT: ADD64ri8 + ; CHECK: JMP_1 %bb.2 + %1:gr64 = ADD64ri8 %5, 4, implicit-def dead $eflags + JMP_1 %bb.2.exit + + bb.2.exit: + $rax = MOV64rr %2 + RET 0 +...