[llvm] 49da20d - Revert 30e8f80fd5a4 "[DebugInfo] Don't create multiple DBG_VALUEs when sinking"
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 10 10:20:43 PST 2019
Author: Hans Wennborg
Date: 2019-12-10T19:20:11+01:00
New Revision: 49da20ddb4319f3f469499e341a1bc3101adcdcf
URL: https://github.com/llvm/llvm-project/commit/49da20ddb4319f3f469499e341a1bc3101adcdcf
DIFF: https://github.com/llvm/llvm-project/commit/49da20ddb4319f3f469499e341a1bc3101adcdcf.diff
LOG: Revert 30e8f80fd5a4 "[DebugInfo] Don't create multiple DBG_VALUEs when sinking"
This caused non-determinism in the compiler, see command on the Phabricator
code review.
> This patch addresses a performance problem reported in PR43855, and
> present in the reapplication in in 001574938e5. It turns out that
> MachineSink will (often) move instructions to the first block that
> post-dominates the current block, and then try to sink further. This
> means if we have a lot of conditionals, we can needlessly create large
> numbers of DBG_VALUEs, one in each block the sunk instruction passes
> through.
>
> To fix this, rather than immediately sinking DBG_VALUEs, record them in
> a pass structure. When sinking is complete and instructions won't be
> sunk any further, new DBG_VALUEs are added, avoiding lots of
> intermediate DBG_VALUE $noregs being created.
>
> Differential revision: https://reviews.llvm.org/D70676
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 2a54e0e380c5..6900c195e0e1 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -15,7 +15,6 @@
//
//===----------------------------------------------------------------------===//
-#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/PointerIntPair.h"
#include "llvm/ADT/SetVector.h"
@@ -128,24 +127,6 @@ namespace {
/// current block.
DenseSet<DebugVariable> SeenDbgVars;
- /// A set of DBG_VALUEs, indexed by their DebugVariable. Refers to the
- /// original / non-sunk DBG_VALUE, which should be cloned to the final
- /// sunk location.
- using SinkingVarSet = DenseMap<DebugVariable, MachineInstr *>;
-
- /// Record of variable locations to re-create after the sinking of a
- /// vreg definition completes.
- struct SunkDebugDef {
- SinkingVarSet InstsToSink; /// Set of DBG_VALUEs to recreate.
- MachineInstr *MI; /// Location to place DBG_VALUEs.
- };
-
- /// Map sunk vreg to the final destination of the vreg def, and the
- /// DBG_VALUEs that were attached to it. We need only create one new
- /// DBG_VALUE even if the defining instruction sinks through multiple
- /// blocks.
- DenseMap<unsigned, SunkDebugDef> SunkDebugDefs;
-
public:
static char ID; // Pass identification
@@ -203,11 +184,6 @@ namespace {
/// to the copy source.
void SalvageUnsunkDebugUsersOfCopy(MachineInstr &,
MachineBasicBlock *TargetBlock);
-
- /// Re-insert any DBG_VALUE instructions that had their operands sunk
- /// in this function.
- void reinsertSunkDebugDefs(MachineFunction &MF);
-
bool AllUsesDominatedByBlock(unsigned Reg, MachineBasicBlock *MBB,
MachineBasicBlock *DefMBB,
bool &BreakPHIEdge, bool &LocalUse) const;
@@ -340,33 +316,6 @@ MachineSinking::AllUsesDominatedByBlock(unsigned Reg,
return true;
}
-void MachineSinking::reinsertSunkDebugDefs(MachineFunction &MF) {
- // Re-insert any DBG_VALUEs sunk.
- for (auto &Iterator : SunkDebugDefs) {
- SunkDebugDef Def = Iterator.second;
- unsigned VReg = Iterator.first;
-
- MachineBasicBlock::iterator DestLoc = Def.MI->getIterator();
- MachineBasicBlock *MBB = Def.MI->getParent();
-
- // Place DBG_VALUEs immediately after the sunk instruction.
- ++DestLoc;
-
- // Collect the DBG_VALUEs being sunk and put them in a deterministic order.
- using VarInstPair = std::pair<DebugVariable, MachineInstr *>;
- SmallVector<VarInstPair, 16> InstsToSink;
- for (auto &SunkLoc : Def.InstsToSink)
- InstsToSink.push_back(SunkLoc);
- llvm::sort(InstsToSink);
-
- for (auto &SunkLoc : InstsToSink) {
- MachineInstr *NewDbgMI = MF.CloneMachineInstr(SunkLoc.second);
- NewDbgMI->getOperand(0).setReg(VReg);
- MBB->insert(DestLoc, NewDbgMI);
- }
- }
-}
-
bool MachineSinking::runOnMachineFunction(MachineFunction &MF) {
if (skipFunction(MF.getFunction()))
return false;
@@ -417,9 +366,6 @@ bool MachineSinking::runOnMachineFunction(MachineFunction &MF) {
MRI->clearKillFlags(I);
RegsToClearKillFlags.clear();
- reinsertSunkDebugDefs(MF);
- SunkDebugDefs.clear();
-
return EverMadeChange;
}
@@ -1037,34 +983,25 @@ bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
++InsertPos;
// Collect debug users of any vreg that this inst defines.
+ SmallVector<MachineInstr *, 4> DbgUsersToSink;
for (auto &MO : MI.operands()) {
if (!MO.isReg() || !MO.isDef() || !MO.getReg().isVirtual())
continue;
-
- // If no DBG_VALUE uses this reg, skip further analysis.
if (!SeenDbgUsers.count(MO.getReg()))
continue;
- SunkDebugDef &SunkDef = SunkDebugDefs[MO.getReg()];
- SunkDef.MI = &MI;
-
- // Record that these DBG_VALUEs refer to this sinking vreg, so that they
- // can be re-specified after sinking completes. Try copy-propagating
- // the original location too.
+ // Sink any users that don't pass any other DBG_VALUEs for this variable.
auto &Users = SeenDbgUsers[MO.getReg()];
for (auto &User : Users) {
MachineInstr *DbgMI = User.getPointer();
- DebugVariable Var(DbgMI->getDebugVariable(), DbgMI->getDebugExpression(),
- DbgMI->getDebugLoc()->getInlinedAt());
- // If we can't copy-propagate the original DBG_VALUE, mark it undef, as
- // its operand will not be available.
- if (!attemptDebugCopyProp(MI, *DbgMI))
- DbgMI->getOperand(0).setReg(0);
-
- // Debug users that don't pass any other DBG_VALUEs for this variable
- // can be sunk.
- if (!User.getInt())
- SunkDef.InstsToSink.insert({Var, DbgMI});
+ 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);
+ }
}
}
@@ -1074,7 +1011,6 @@ bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
if (MI.getMF()->getFunction().getSubprogram() && MI.isCopy())
SalvageUnsunkDebugUsersOfCopy(MI, SuccToSinkTo);
- SmallVector<MachineInstr *, 4> DbgUsersToSink; // Deliberately empty
performSink(MI, *SuccToSinkTo, InsertPos, DbgUsersToSink);
// Conservatively, clear any kill flags, since it's possible that they are no
diff --git a/llvm/test/DebugInfo/MIR/X86/machinesink.mir b/llvm/test/DebugInfo/MIR/X86/machinesink.mir
index 25da8753f761..05f3743c1f2d 100644
--- a/llvm/test/DebugInfo/MIR/X86/machinesink.mir
+++ b/llvm/test/DebugInfo/MIR/X86/machinesink.mir
@@ -9,9 +9,6 @@
# 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.
-#
-# Test four: if we sink a DBG_VALUE via an intermediate block, no spurious
-# DBG_VALUE $noreg's should appear along the way.
--- |
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
@@ -49,22 +46,6 @@
ret void
}
- define void @test4(i32* nocapture readonly %p) local_unnamed_addr !dbg !301 {
- ; Stripped
- entry:
- br label %block1
- block1:
- br label %block2
- block2:
- br label %exit
- combined:
- br label %branch1
- branch1:
- br label %exit
- exit:
- ret void
- }
-
; Function Attrs: nounwind readnone
declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
@@ -100,15 +81,10 @@
!202 = !{!203}
!203 = !DILocalVariable(name: "r", arg: 1, scope: !201, file: !2, line: 2, type: !12)
!204 = !DILocation(line: 1, column: 1, scope: !201)
- !301 = 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: !302)
- !302 = !{!303}
- !303 = !DILocalVariable(name: "s", arg: 1, scope: !301, file: !2, line: 2, type: !12)
- !304 = !DILocation(line: 1, column: 1, scope: !301)
; CHECK: [[VARNUM:![0-9]+]] = !DILocalVariable(name: "p",
; CHECK: [[VAR2NUM:![0-9]+]] = !DILocalVariable(name: "q",
; CHECK: [[VAR3NUM:![0-9]+]] = !DILocalVariable(name: "r",
- ; CHECK: [[VAR4NUM:![0-9]+]] = !DILocalVariable(name: "s",
...
---
@@ -259,85 +235,3 @@ body: |
$rax = MOV64rr %2
RET 0
...
----
-name: test4
-tracksRegLiveness: true
-liveins:
- - { reg: '$rdi', virtual-reg: '%2' }
- - { reg: '$rsi', virtual-reg: '%2' }
-body: |
- bb.0.entry:
- successors: %bb.1.block1, %bb.2.block2
- liveins: $rdi, $esi
-
- ; The DBG_VALUE here should sink through several blocks, but not leave any
- ; additional DBG_VALUE $noregs on its way.
- ; CHECK-LABEL: bb.0.entry:
- ; CHECK: [[TEST4VREG:%[0-9]+]]:gr64 = COPY $rdi
- ; CHECK-NEXT: CMP32ri $esi, 0
- ; CHECK-NEXT: DBG_VALUE $noreg, $noreg, [[VAR4NUM]]
- ; 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, !303, !17, debug-location !304
- JCC_1 %bb.1.block1, 4, implicit $eflags
- JMP_1 %bb.2.block2
-
- bb.1.block1:
- successors: %bb.3.combined
- liveins: $esi
-
- ; CHECK-LABEL: bb.1.block1
- ; CHECK-NOT: DBG_VALUE
- ; CHECK: JMP_1 %bb.3
-
- JMP_1 %bb.3.combined
-
- bb.2.block2:
- successors: %bb.3.combined
- liveins: $esi
-
- ; CHECK-LABEL: bb.2.block2
- ; CHECK-NOT: DBG_VALUE
- ; CHECK: JMP_1 %bb.3
-
- JMP_1 %bb.3.combined
-
- bb.3.combined:
- successors: %bb.4.branch1, %bb.5.exit
- liveins: $esi
-
- ; CHECK-LABEL: bb.3.combined
- ; CHECK-NOT: DBG_VALUE
- ; CHECK: JCC_1 %bb.4, 4, implicit $eflags
- ; CHECK-NEXT: JMP_1 %bb.5
-
- CMP32ri $esi, 1, implicit-def $eflags
- JCC_1 %bb.4.branch1, 4, implicit $eflags
- JMP_1 %bb.5.exit
-
- bb.4.branch1:
- successors: %bb.5.exit
-
- ; This block should receive the sunk copy and DBG_VALUE.
- ; CHECK-LABEL: bb.4.branch1:
- ; CHECK-NOT: DBG_VALUE
- ; CHECK: [[TEST4VREG2:%[0-9]+]]:gr64 = ADD64ri8 [[TEST4VREG]], 1
- ; CHECK-NEXT: DBG_VALUE [[TEST4VREG2]], $noreg, [[VAR4NUM]]
- ; CHECK-NEXT: ADD64ri8
- ; CHECK: JMP_1 %bb.5
-
- %1:gr64 = ADD64ri8 %5, 4, implicit-def dead $eflags
- JMP_1 %bb.5.exit
-
- bb.5.exit:
- ; CHECK-LABEL: bb.5.exit
- ; CHECK-NOT: DBG_VALUE
- ; CHECK: RET 0
-
- $rax = MOV64rr %2
- RET 0
-...
More information about the llvm-commits
mailing list