[llvm] d382a8a - Revert "[DebugInfo] MachineSink: find more DBG_VALUEs to sink"
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 31 05:39:23 PDT 2019
Author: Jeremy Morse
Date: 2019-10-31T12:39:06Z
New Revision: d382a8a768b3636c5aa1a934977c54d0215633cf
URL: https://github.com/llvm/llvm-project/commit/d382a8a768b3636c5aa1a934977c54d0215633cf
DIFF: https://github.com/llvm/llvm-project/commit/d382a8a768b3636c5aa1a934977c54d0215633cf.diff
LOG: Revert "[DebugInfo] MachineSink: find more DBG_VALUEs to sink"
This reverts commit f5e1b718a675a4449b71423f04d38e1e93045105.
PR43855 reports a performance regression with commit ee50590e. This commit
depends on the faulty one, so has to come out too.
Added:
Modified:
llvm/lib/CodeGen/MachineSink.cpp
Removed:
llvm/test/DebugInfo/MIR/X86/machinesink.mir
################################################################################
diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index 81fd30ca0529..4c55158a0048 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -105,9 +105,6 @@ 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;
-
public:
static char ID; // Pass identification
@@ -135,7 +132,6 @@ namespace {
private:
bool ProcessBlock(MachineBasicBlock &MBB);
- void ProcessDbgInst(MachineInstr &MI);
bool isWorthBreakingCriticalEdge(MachineInstr &MI,
MachineBasicBlock *From,
MachineBasicBlock *To);
@@ -157,14 +153,8 @@ namespace {
MachineBasicBlock *To,
bool BreakPHIEdge);
bool SinkInstruction(MachineInstr &MI, bool &SawStore,
- AllSuccsCache &AllSuccessors);
- /// If we sink a COPY inst, some debug users of it's destination may no
- /// longer be dominated by the COPY, and will eventually be dropped.
- /// This is easily rectified by forwarding the non-dominated debug uses
- /// to the copy source.
- void SalvageUnsunkDebugUsersOfCopy(MachineInstr &,
- MachineBasicBlock *TargetBlock);
+ AllSuccsCache &AllSuccessors);
bool AllUsesDominatedByBlock(unsigned Reg, MachineBasicBlock *MBB,
MachineBasicBlock *DefMBB,
bool &BreakPHIEdge, bool &LocalUse) const;
@@ -377,11 +367,8 @@ bool MachineSinking::ProcessBlock(MachineBasicBlock &MBB) {
if (!ProcessedBegin)
--I;
- if (MI.isDebugInstr()) {
- if (MI.isDebugValue())
- ProcessDbgInst(MI);
+ if (MI.isDebugInstr())
continue;
- }
bool Joined = PerformTrivialForwardCoalescing(MI, &MBB);
if (Joined) {
@@ -397,23 +384,9 @@ bool MachineSinking::ProcessBlock(MachineBasicBlock &MBB) {
// If we just processed the first instruction in the block, we're done.
} while (!ProcessedBegin);
- SeenDbgUsers.clear();
-
return MadeChange;
}
-void MachineSinking::ProcessDbgInst(MachineInstr &MI) {
- // When we see DBG_VALUEs for registers, record any vreg it reads, so that
- // we know what to sink if the vreg def sinks.
- assert(MI.isDebugValue() && "Expected DBG_VALUE for processing");
-
- MachineOperand &MO = MI.getOperand(0);
- if (!MO.isReg() || !MO.getReg().isVirtual())
- return;
-
- SeenDbgUsers[MO.getReg()].push_back(&MI);
-}
-
bool MachineSinking::isWorthBreakingCriticalEdge(MachineInstr &MI,
MachineBasicBlock *From,
MachineBasicBlock *To) {
@@ -758,13 +731,22 @@ static bool SinkingPreventsImplicitNullCheck(MachineInstr &MI,
MBP.LHS.getReg() == BaseOp->getReg();
}
-/// Sink an instruction and its associated debug instructions.
+/// Sink an instruction and its associated debug instructions. If the debug
+/// instructions to be sunk are already known, they can be provided in DbgVals.
static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo,
MachineBasicBlock::iterator InsertPos,
- SmallVectorImpl<MachineInstr *> &DbgValuesToSink) {
+ SmallVectorImpl<MachineInstr *> *DbgVals = nullptr) {
const MachineRegisterInfo &MRI = MI.getMF()->getRegInfo();
const TargetInstrInfo &TII = *MI.getMF()->getSubtarget().getInstrInfo();
+ // If debug values are provided use those, otherwise call collectDebugValues.
+ SmallVector<MachineInstr *, 2> DbgValuesToSink;
+ if (DbgVals)
+ DbgValuesToSink.insert(DbgValuesToSink.begin(),
+ DbgVals->begin(), DbgVals->end());
+ else
+ MI.collectDebugValues(DbgValuesToSink);
+
// 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
// wrong location information.
@@ -947,25 +929,7 @@ bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
while (InsertPos != SuccToSinkTo->end() && InsertPos->isPHI())
++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 (!SeenDbgUsers.count(MO.getReg()))
- continue;
-
- auto &Users = SeenDbgUsers[MO.getReg()];
- DbgUsersToSink.insert(DbgUsersToSink.end(), Users.begin(), Users.end());
- }
-
- // After sinking, some debug users may not be dominated any more. If possible,
- // copy-propagate their operands. As it's expensive, don't do this if there's
- // no debuginfo in the program.
- if (MI.getMF()->getFunction().getSubprogram() && MI.isCopy())
- SalvageUnsunkDebugUsersOfCopy(MI, SuccToSinkTo);
-
- performSink(MI, *SuccToSinkTo, InsertPos, DbgUsersToSink);
+ performSink(MI, *SuccToSinkTo, InsertPos);
// Conservatively, clear any kill flags, since it's possible that they are no
// longer correct.
@@ -980,41 +944,6 @@ bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
return true;
}
-void MachineSinking::SalvageUnsunkDebugUsersOfCopy(
- MachineInstr &MI, MachineBasicBlock *TargetBlock) {
- assert(MI.isCopy());
- assert(MI.getOperand(1).isReg());
-
- // Enumerate all users of vreg operands that are def'd. Skip those that will
- // be sunk. For the rest, if they are not dominated by the block we will sink
- // MI into, propagate the copy source to them.
- SmallVector<MachineInstr *, 4> DbgDefUsers;
- const MachineRegisterInfo &MRI = MI.getMF()->getRegInfo();
- for (auto &MO : MI.operands()) {
- if (!MO.isReg() || !MO.isDef() || !MO.getReg().isVirtual())
- continue;
- for (auto &User : MRI.use_instructions(MO.getReg())) {
- if (!User.isDebugValue() || DT->dominates(TargetBlock, User.getParent()))
- continue;
-
- // If is in same block, will either sink or be use-before-def.
- if (User.getParent() == MI.getParent())
- continue;
-
- assert(User.getOperand(0).isReg() &&
- "DBG_VALUE user of vreg, but non reg operand?");
- DbgDefUsers.push_back(&User);
- }
- }
-
- // Point the users of this copy that are no longer dominated, at the source
- // of the copy.
- for (auto *User : DbgDefUsers) {
- User->getOperand(0).setReg(MI.getOperand(1).getReg());
- User->getOperand(0).setSubReg(MI.getOperand(1).getSubReg());
- }
-}
-
//===----------------------------------------------------------------------===//
// This pass is not intended to be a replacement or a complete alternative
// for the pre-ra machine sink pass. It is only designed to sink COPY
@@ -1324,7 +1253,7 @@ bool PostRAMachineSinking::tryToSinkCopy(MachineBasicBlock &CurBB,
// block.
clearKillFlags(MI, CurBB, UsedOpsInCopy, UsedRegUnits, TRI);
MachineBasicBlock::iterator InsertPos = SuccBB->getFirstNonPHI();
- performSink(*MI, *SuccBB, InsertPos, DbgValsToSink);
+ performSink(*MI, *SuccBB, InsertPos, &DbgValsToSink);
updateLiveIn(MI, SuccBB, UsedOpsInCopy, DefedRegsInCopy);
Changed = true;
diff --git a/llvm/test/DebugInfo/MIR/X86/machinesink.mir b/llvm/test/DebugInfo/MIR/X86/machinesink.mir
deleted file mode 100644
index 3df80ee15f51..000000000000
--- a/llvm/test/DebugInfo/MIR/X86/machinesink.mir
+++ /dev/null
@@ -1,105 +0,0 @@
-# RUN: llc -mtriple=x86_64-unknown-unknown -run-pass=machine-sink -o - %s | FileCheck %s
-# 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.
---- |
- target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
- target triple = "x86_64-unknown-linux-gnu"
-
- @x = common local_unnamed_addr global i32 0, align 4, !dbg !0
-
- ; Function Attrs: noreturn nounwind uwtable
- define void @Process(i32* nocapture readonly %p) local_unnamed_addr !dbg !9 {
- ; Stripped
- entry:
- br label %nou
- nou:
- br label %exit
- exit:
- ret void
- }
-
- ; Function Attrs: nounwind readnone
- declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
-
- !llvm.dbg.cu = !{!1}
- !llvm.module.flags = !{!6, !7}
- !llvm.ident = !{!8}
-
- !0 = !DIGlobalVariableExpression(var: !DIGlobalVariable(name: "x", scope: !1, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true), expr: !DIExpression())
- !1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !3, globals: !4)
- !2 = !DIFile(filename: "t.c", directory: "")
- !3 = !{}
- !4 = !{!0}
- !5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
- !6 = !{i32 2, !"Dwarf Version", i32 4}
- !7 = !{i32 2, !"Debug Info Version", i32 3}
- !8 = !{!"clang version 4.0.0 "}
- !9 = distinct !DISubprogram(name: "Process", scope: !2, file: !2, line: 2, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !1, retainedNodes: !15)
- !10 = !DISubroutineType(types: !11)
- !11 = !{null, !12}
- !12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
- !13 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !14)
- !14 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
- !15 = !{!16}
- !16 = !DILocalVariable(name: "p", arg: 1, scope: !9, file: !2, line: 2, type: !12)
- !17 = !DIExpression()
- !18 = !DILocation(line: 2, column: 34, scope: !9)
- !28 = !DILexicalBlockFile(scope: !9, file: !2, discriminator: 1)
-
- ; CHECK: [[VARNUM:![0-9]+]] = !DILocalVariable(name: "p",
-
-...
----
-name: Process
-tracksRegLiveness: true
-liveins:
- - { reg: '$rdi', virtual-reg: '%2' }
- - { reg: '$rsi', virtual-reg: '%2' }
-body: |
- bb.0.entry:
- successors: %bb.1.nou(0x80000000), %bb.2.exit
- liveins: $rdi, $esi
-
- ; This block should have the vreg copy sunk from it, the DBG_VALUE with it,
- ; and a copy-prop'd DBG_VALUE left over.
- ; CHECK-LABEL: bb.0.entry:
- ; CHECK: [[ARG0VREG:%[0-9]+]]:gr64 = COPY $rdi
- ; CHECK-NEXT: CMP32ri $esi, 0
- ; CHECK-NEXT: DBG_VALUE [[ARG0VREG]], $noreg, [[VARNUM]]
- ; 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, !16, !17, debug-location !18
- JCC_1 %bb.1.nou, 4, implicit $eflags
- JMP_1 %bb.2.exit
-
- bb.1.nou:
- successors: %bb.2.exit(0x80000000)
-
- ; This block should receive the sunk copy and DBG_VALUE
- ; CHECK-LABEL: bb.1.nou:
- ; CHECK: [[SUNKVREG:%[0-9]+]]:gr64 = COPY [[ARG0VREG]]
- ; CHECK-NEXT: DBG_VALUE [[SUNKVREG]], $noreg, [[VARNUM]]
- ; CHECK-NEXT: ADD64ri8
- ; CHECK-NEXT: JMP_1
- %1:gr64 = ADD64ri8 %5, 4, implicit-def dead $eflags
- JMP_1 %bb.2.exit
-
- bb.2.exit:
- ; The DBG_VALUE below should have its operand copy-propagated after
- ; the copy to %5 is sunk.
- ; CHECK-LABEL: bb.2.exit:
- ; CHECK: DBG_VALUE [[ARG0VREG]], $noreg, [[VARNUM]]
- ; CHECK-NEXT: $rax = MOV64rr [[ARG0VREG]]
- ; CHECK-NEXT: RET 0
- DBG_VALUE %5, _, !16, !17, debug-location !18
- $rax = MOV64rr %2
- RET 0
-...
More information about the llvm-commits
mailing list