[DebugInfo][RemoveDIs] Instrument jump-threading to update DPValues #73127
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch makes jump-threading handle non-instruction debug-info stored in DPValues in the same way that it updates dbg.values nowadays. This involves re-targetting their operands as with dbg.values getting moved from one block to another, and manually cloning them when duplicating blocks. The SSAUpdater class also grows some functions for SSA-updating DPValues in the same way as dbg.values. All of this is largely covered by existing debug-info tests, except for the cloning of DPValues attached to elidable instructions and branches, where I've added a test to thread-debug-info.ll. Where previously we could rely on dbg.values being copied and cloned as normal instructions are, as we need to explicitly perform that operation now I've added some explicit testing for it.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesThis patch makes jump-threading handle non-instruction debug-info stored in DPValues in the same way that it updates dbg.values nowadays. This involves re-targetting their operands as with dbg.values getting moved from one block to another, and manually cloning them when duplicating blocks. The SSAUpdater class also grows some functions for SSA-updating DPValues in the same way as dbg.values. All of this is largely covered by existing debug-info tests, except for the cloning of DPValues attached to elidable instructions and branches, where I've added a test to thread-debug-info.ll. Where previously we could rely on dbg.values being copied and cloned as normal instructions are, as we need to explicitly perform that operation now I've added some explicit testing for it. Full diff: https://github.com/llvm/llvm-project/pull/73127.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/SSAUpdater.h b/llvm/include/llvm/Transforms/Utils/SSAUpdater.h
index 36fbf536f6d015c..551fed785cb3c0a 100644
--- a/llvm/include/llvm/Transforms/Utils/SSAUpdater.h
+++ b/llvm/include/llvm/Transforms/Utils/SSAUpdater.h
@@ -23,6 +23,7 @@ class BasicBlock;
class Instruction;
class LoadInst;
class PHINode;
+class DPValue;
template <typename T> class SmallVectorImpl;
template <typename T> class SSAUpdaterTraits;
class Type;
@@ -123,6 +124,8 @@ class SSAUpdater {
void UpdateDebugValues(Instruction *I);
void UpdateDebugValues(Instruction *I,
SmallVectorImpl<DbgValueInst *> &DbgValues);
+ void UpdateDebugValues(Instruction *I,
+ SmallVectorImpl<DPValue *> &DbgValues);
/// Rewrite a use like \c RewriteUse but handling in-block definitions.
///
@@ -134,6 +137,7 @@ class SSAUpdater {
private:
Value *GetValueAtEndOfBlockInternal(BasicBlock *BB);
void UpdateDebugValue(Instruction *I, DbgValueInst *DbgValue);
+ void UpdateDebugValue(Instruction *I, DPValue *DbgValue);
};
/// Helper class for promoting a collection of loads and stores into SSA
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 2d899f100f8154d..d7d503427ec3d20 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -410,6 +410,10 @@ static bool replaceFoldableUses(Instruction *Cond, Value *ToVal,
if (Cond->getParent() == KnownAtEndOfBB)
Changed |= replaceNonLocalUsesWith(Cond, ToVal);
for (Instruction &I : reverse(*KnownAtEndOfBB)) {
+ // Replace any debug-info record users of Cond with ToVal.
+ for (DPValue &DPV : I.getDbgValueRange())
+ DPV.replaceVariableLocationOp(Cond, ToVal, true);
+
// Reached the Cond whose uses we are trying to replace, so there are no
// more uses.
if (&I == Cond)
@@ -1961,6 +1965,7 @@ void JumpThreadingPass::updateSSA(
SSAUpdater SSAUpdate;
SmallVector<Use *, 16> UsesToRename;
SmallVector<DbgValueInst *, 4> DbgValues;
+ SmallVector<DPValue *, 4> DPValues;
for (Instruction &I : *BB) {
// Scan all uses of this instruction to see if it is used outside of its
@@ -1977,10 +1982,13 @@ void JumpThreadingPass::updateSSA(
}
// Find debug values outside of the block
- findDbgValues(DbgValues, &I);
+ findDbgValues(DbgValues, &I, &DPValues);
llvm::erase_if(DbgValues, [&](const DbgValueInst *DbgVal) {
return DbgVal->getParent() == BB;
});
+ llvm::erase_if(DPValues, [&](const DPValue *DPVal) {
+ return DPVal->getParent() == BB;
+ });
// If there are no uses outside the block, we're done with this instruction.
if (UsesToRename.empty() && DbgValues.empty())
@@ -1996,9 +2004,11 @@ void JumpThreadingPass::updateSSA(
while (!UsesToRename.empty())
SSAUpdate.RewriteUse(*UsesToRename.pop_back_val());
- if (!DbgValues.empty()) {
+ if (!DbgValues.empty() || !DPValues.empty()) {
SSAUpdate.UpdateDebugValues(&I, DbgValues);
+ SSAUpdate.UpdateDebugValues(&I, DPValues);
DbgValues.clear();
+ DPValues.clear();
}
LLVM_DEBUG(dbgs() << "\n");
@@ -2041,6 +2051,26 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
return true;
};
+ // Duplicate implementation of the above dbg.value code, using DPValues
+ // instead.
+ auto RetargetDPValueIfPossible = [&](DPValue *DPV) {
+ SmallSet<std::pair<Value *, Value *>, 16> OperandsToRemap;
+ for (auto *Op : DPV->location_ops()) {
+ Instruction *OpInst = dyn_cast<Instruction>(Op);
+ if (!OpInst)
+ continue;
+
+ auto I = ValueMapping.find(OpInst);
+ if (I != ValueMapping.end())
+ OperandsToRemap.insert({OpInst, I->second});
+ }
+
+ for (auto &[OldOp, MappedOp] : OperandsToRemap)
+ DPV->replaceVariableLocationOp(OldOp, MappedOp);
+ };
+
+ BasicBlock *RangeBB = BI->getParent();
+
// Clone the phi nodes of the source basic block into NewBB. The resulting
// phi nodes are trivial since NewBB only has one predecessor, but SSAUpdater
// might need to rewrite the operand of the cloned phi.
@@ -2059,6 +2089,12 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
identifyNoAliasScopesToClone(BI, BE, NoAliasScopes);
cloneNoAliasScopes(NoAliasScopes, ClonedScopes, "thread", Context);
+ auto CloneAndRemapDbgInfo = [&](Instruction *NewInst, Instruction *From) {
+ auto DPVRange = NewInst->cloneDebugInfoFrom(From);
+ for (DPValue &DPV : DPVRange)
+ RetargetDPValueIfPossible(&DPV);
+ };
+
// Clone the non-phi instructions of the source basic block into NewBB,
// keeping track of the mapping and using it to remap operands in the cloned
// instructions.
@@ -2069,6 +2105,8 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
ValueMapping[&*BI] = New;
adaptNoAliasScopes(New, ClonedScopes, Context);
+ CloneAndRemapDbgInfo(New, &*BI);
+
if (RetargetDbgValueIfPossible(New))
continue;
@@ -2081,6 +2119,17 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
}
}
+ // There may be DPValues on the terminator, clone directly from marker
+ // to marker as there isn't an instruction there.
+ if (BE != RangeBB->end() && BE->hasDbgValues()) {
+ // Dump them at the end.
+ DPMarker *Marker = RangeBB->getMarker(BE);
+ DPMarker *EndMarker = NewBB->createMarker(NewBB->end());
+ auto DPVRange = EndMarker->cloneDebugInfoFrom(Marker, std::nullopt);
+ for (DPValue &DPV : DPVRange)
+ RetargetDPValueIfPossible(&DPV);
+ }
+
return ValueMapping;
}
@@ -2666,6 +2715,9 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
if (!New->mayHaveSideEffects()) {
New->eraseFromParent();
New = nullptr;
+ // Clone debug-info on the elided instruction to the destination
+ // position.
+ OldPredBranch->cloneDebugInfoFrom(&*BI, std::nullopt, true);
}
} else {
ValueMapping[&*BI] = New;
@@ -2673,6 +2725,8 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
if (New) {
// Otherwise, insert the new instruction into the block.
New->setName(BI->getName());
+ // Clone across any debug-info attached to the old instruction.
+ New->cloneDebugInfoFrom(&*BI);
// Update Dominance from simplified New instruction operands.
for (unsigned i = 0, e = New->getNumOperands(); i != e; ++i)
if (BasicBlock *SuccBB = dyn_cast<BasicBlock>(New->getOperand(i)))
diff --git a/llvm/lib/Transforms/Utils/SSAUpdater.cpp b/llvm/lib/Transforms/Utils/SSAUpdater.cpp
index 794c0258cca27a0..eda9d17296cb75c 100644
--- a/llvm/lib/Transforms/Utils/SSAUpdater.cpp
+++ b/llvm/lib/Transforms/Utils/SSAUpdater.cpp
@@ -199,12 +199,18 @@ void SSAUpdater::RewriteUse(Use &U) {
void SSAUpdater::UpdateDebugValues(Instruction *I) {
SmallVector<DbgValueInst *, 4> DbgValues;
- llvm::findDbgValues(DbgValues, I);
+ SmallVector<DPValue *, 4> DPValues;
+ llvm::findDbgValues(DbgValues, I, &DPValues);
for (auto &DbgValue : DbgValues) {
if (DbgValue->getParent() == I->getParent())
continue;
UpdateDebugValue(I, DbgValue);
}
+ for (auto &DPV: DPValues) {
+ if (DPV->getParent() == I->getParent())
+ continue;
+ UpdateDebugValue(I, DPV);
+ }
}
void SSAUpdater::UpdateDebugValues(Instruction *I,
@@ -214,6 +220,13 @@ void SSAUpdater::UpdateDebugValues(Instruction *I,
}
}
+void SSAUpdater::UpdateDebugValues(Instruction *I,
+ SmallVectorImpl<DPValue *> &DPValues) {
+ for (auto &DPV : DPValues) {
+ UpdateDebugValue(I, DPV);
+ }
+}
+
void SSAUpdater::UpdateDebugValue(Instruction *I, DbgValueInst *DbgValue) {
BasicBlock *UserBB = DbgValue->getParent();
if (HasValueForBlock(UserBB)) {
@@ -224,6 +237,16 @@ void SSAUpdater::UpdateDebugValue(Instruction *I, DbgValueInst *DbgValue) {
DbgValue->setKillLocation();
}
+void SSAUpdater::UpdateDebugValue(Instruction *I, DPValue *DPV) {
+ BasicBlock *UserBB = DPV->getParent();
+ if (HasValueForBlock(UserBB)) {
+ Value *NewVal = GetValueAtEndOfBlock(UserBB);
+ DPV->replaceVariableLocationOp(I, NewVal);
+ }
+ else
+ DPV->setKillLocation();
+}
+
void SSAUpdater::RewriteUseAfterInsertions(Use &U) {
Instruction *User = cast<Instruction>(U.getUser());
diff --git a/llvm/test/Transforms/JumpThreading/redundant-dbg-info.ll b/llvm/test/Transforms/JumpThreading/redundant-dbg-info.ll
index 7659d72babf7a42..8aaed55788c5ce6 100644
--- a/llvm/test/Transforms/JumpThreading/redundant-dbg-info.ll
+++ b/llvm/test/Transforms/JumpThreading/redundant-dbg-info.ll
@@ -1,4 +1,5 @@
; RUN: opt -passes=jump-threading -S < %s | FileCheck %s
+; RUN: opt -passes=jump-threading -S < %s --try-experimental-debuginfo-iterators | FileCheck %s
define dso_local i32 @_Z3fooi(i32 %a) !dbg !7 {
entry:
diff --git a/llvm/test/Transforms/JumpThreading/thread-debug-info.ll b/llvm/test/Transforms/JumpThreading/thread-debug-info.ll
index 03478292779c3ad..531f7371870cb10 100644
--- a/llvm/test/Transforms/JumpThreading/thread-debug-info.ll
+++ b/llvm/test/Transforms/JumpThreading/thread-debug-info.ll
@@ -1,4 +1,5 @@
; RUN: opt -S -passes=jump-threading < %s | FileCheck %s
+; RUN: opt -S -passes=jump-threading < %s --try-experimental-debuginfo-iterators | FileCheck %s
@a = global i32 0, align 4
; Test that the llvm.dbg.value calls in a threaded block are correctly updated to
@@ -91,6 +92,47 @@ exit: ; preds = %bb.f4, %bb.f3, %bb.
ret void, !dbg !29
}
+; Test for the cloning of dbg.values on elided instructions -- down one path
+; being threaded, the and in the function below is optimised away, but it's
+; debug-info should still be preserved.
+; Similarly, the call to f1 gets cloned, it's dbg.value should be cloned too.
+define void @test16(i1 %c, i1 %c2, i1 %c3, i1 %c4) nounwind ssp !dbg !30 {
+; CHECK-LABEL: define void @test16(i1
+entry:
+ %cmp = icmp sgt i32 undef, 1, !dbg !33
+ br i1 %c, label %land.end, label %land.rhs, !dbg !33
+
+land.rhs:
+ br i1 %c2, label %lor.lhs.false.i, label %land.end, !dbg !33
+
+lor.lhs.false.i:
+ br i1 %c3, label %land.end, label %land.end, !dbg !33
+
+; CHECK-LABEL: land.end.thr_comm:
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 0,
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 0,
+; CHECK-NEXT: call void @f1()
+; CHECK-NEXT: br i1 %c4,
+
+; CHECK-LABEL: land.end:
+; CHECK-NEXT: %0 = phi i1
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 0,
+land.end:
+ %0 = phi i1 [ true, %entry ], [ false, %land.rhs ], [false, %lor.lhs.false.i], [false, %lor.lhs.false.i]
+ call void @llvm.dbg.value(metadata i32 0, metadata !32, metadata !DIExpression()), !dbg !33
+ %cmp12 = and i1 %cmp, %0, !dbg !33
+ %xor1 = xor i1 %cmp12, %c4, !dbg !33
+ call void @llvm.dbg.value(metadata i32 0, metadata !32, metadata !DIExpression()), !dbg !33
+ call void @f1()
+ br i1 %xor1, label %if.then, label %if.end, !dbg !33
+
+if.then:
+ ret void, !dbg !33
+
+if.end:
+ ret void, !dbg !33
+}
+
declare void @f1()
declare void @f2()
@@ -138,3 +180,7 @@ attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memo
!27 = !DILocation(line: 13, column: 1, scope: !5)
!28 = !DILocation(line: 14, column: 1, scope: !5)
!29 = !DILocation(line: 15, column: 1, scope: !5)
+!30 = distinct !DISubprogram(name: "test13", linkageName: "test13", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !31)
+!31 = !{!32}
+!32 = !DILocalVariable(name: "1", scope: !30, file: !1, line: 1, type: !10)
+!33 = !DILocation(line: 1, column: 1, scope: !30)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
OCHyams
approved these changes
Nov 23, 2023
| @@ -410,6 +410,10 @@ static bool replaceFoldableUses(Instruction *Cond, Value *ToVal, | |||
| if (Cond->getParent() == KnownAtEndOfBB) | |||
| Changed |= replaceNonLocalUsesWith(Cond, ToVal); | |||
| for (Instruction &I : reverse(*KnownAtEndOfBB)) { | |||
| // Replace any debug-info record users of Cond with ToVal. | |||
| for (DPValue &DPV : I.getDbgValueRange()) | |||
| DPV.replaceVariableLocationOp(Cond, ToVal, true); | |||
There was a problem hiding this comment.
Is the AllowEmpty (3rd parameter) situation something to do with use-before-defs?
There was a problem hiding this comment.
It's more of a check-my-errors flag -- using replaceVariableLocationOp on debug intrinsics that don't use the "Source" value is considered an error, and there's an assertion for that. Wheras this code here is speculatively trying to "replace this value if it's used, never mind if it isn't", and the AllowEmpty parameter signals that there shouldn't be an error if the Source value can't be found.
There are a bunch of different ways of doing this, I'll admit I haven't thought about how to restructure it, but it's an existing API call on DbgValueInst, so I didn't want to get too far away from that.
| @@ -91,6 +92,47 @@ exit: ; preds = %bb.f4, %bb.f3, %bb. | |||
| ret void, !dbg !29 | |||
| } | |||
|
|
|||
| ; Test for the cloning of dbg.values on elided instructions -- down one path | |||
| ; being threaded, the and in the function below is optimised away, but it's | |||
There was a problem hiding this comment.
Could you demark "and" so it doesn't look like a word, e.g. `and`?
nit: it's -> its (apostrophe is possessive unless there's already a contraction, I think is the rule. At the very least, "it's" is always "it is" and never "belonging to it").
There's another "it's" in the next sentence that wants the same treatment.
| br i1 %c3, label %land.end, label %land.end, !dbg !33 | ||
|
|
||
| ; CHECK-LABEL: land.end.thr_comm: | ||
| ; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 0, |
There was a problem hiding this comment.
Can you give the dbg.values different constant values to easily identify which is which in the output/CHECK lines?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch makes jump-threading handle non-instruction debug-info stored in DPValues in the same way that it updates dbg.values nowadays. This involves re-targetting their operands as with dbg.values getting moved from one block to another, and manually cloning them when duplicating blocks. The SSAUpdater class also grows some functions for SSA-updating DPValues in the same way as dbg.values.
All of this is largely covered by existing debug-info tests, except for the cloning of DPValues attached to elidable instructions and branches, where I've added a test to thread-debug-info.ll. Where previously we could rely on dbg.values being copied and cloned as normal instructions are, as we need to explicitly perform that operation now I've added some explicit testing for it.