[llvm] e4cdd62 - [DebugInfo] Don't reorder DBG_VALUEs when sunk
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 5 07:54:43 PST 2019
Author: Jeremy Morse
Date: 2019-12-05T15:52:20Z
New Revision: e4cdd6263175f7289cfb61608944892d8c76b6ff
URL: https://github.com/llvm/llvm-project/commit/e4cdd6263175f7289cfb61608944892d8c76b6ff
DIFF: https://github.com/llvm/llvm-project/commit/e4cdd6263175f7289cfb61608944892d8c76b6ff.diff
LOG: [DebugInfo] Don't reorder DBG_VALUEs when sunk
Fix part of PR43855, resolving a problem that comes from the reapplication
in 001574938e5. If we have two DBG_VALUE insts in a block that specify
the location of the same variable, for example:
%0 = someinst
DBG_VALUE %0, !123, !DIExpression()
%1 = anotherinst
DBG_VALUE %1, !123, !DIExpression()
if %0 were to sink, the corresponding DBG_VALUE would sink too, past the
next DBG_VALUE, effectively re-ordering assignments. To fix this, I've
added a SeenDbgVars set recording what variable locations have been seen in
a block already (working bottom up), and now flag DBG_VALUEs that would
pass a later DBG_VALUE for the same variable.
NB, this only works for repeated DBG_VALUEs in the same basic block, the
general case involving control flow is much harder, which I've written
up in PR44117.
Differential revision: https://reviews.llvm.org/D70672
Added:
Modified:
llvm/lib/CodeGen/MachineSink.cpp
llvm/test/DebugInfo/MIR/X86/machinesink.mir
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index f34e820a2e04..6900c195e0e1 100644
--- 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 @@ namespace {
using AllSuccsCache =
std::map<MachineBasicBlock *, SmallVector<MachineBasicBlock *, 4>>;
- // Remember debug uses of vregs seen, so we know what to sink out of blocks.
- DenseMap<unsigned, TinyPtrVector<MachineInstr *>> 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<MachineInstr *, 1>;
+
+ /// Record of DBG_VALUE uses of vregs in a block, so that we can identify
+ /// debug instructions to sink.
+ SmallDenseMap<unsigned, TinyPtrVector<SeenDbgUser>> SeenDbgUsers;
+
+ /// Record of debug variables that have had their locations set in the
+ /// current block.
+ DenseSet<DebugVariable> SeenDbgVars;
public:
static char ID; // Pass identification
@@ -399,6 +417,7 @@ bool MachineSinking::ProcessBlock(MachineBasicBlock &MBB) {
} while (!ProcessedBegin);
SeenDbgUsers.clear();
+ SeenDbgVars.clear();
return MadeChange;
}
@@ -408,11 +427,16 @@ void MachineSinking::ProcessDbgInst(MachineInstr &MI) {
// 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 @@ static bool SinkingPreventsImplicitNullCheck(MachineInstr &MI,
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<MachineInstr *> &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 @@ static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo,
// 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<MachineInstr *>::iterator DBI = DbgValuesToSink.begin(),
DBE = DbgValuesToSink.end();
DBI != DBE; ++DBI) {
@@ -794,46 +863,8 @@ static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo,
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 @@ bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
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
index 3df80ee15f51..05f3743c1f2d 100644
--- 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
diff erent 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 @@ body: |
$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
+...
More information about the llvm-commits
mailing list