Skip to content

[DebugInfo][RemoveDIs] Instrument jump-threading to update DPValues #73127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 23, 2023
Merged

[DebugInfo][RemoveDIs] Instrument jump-threading to update DPValues #73127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
effd2c3
[DebugInfo][RemoveDIs] Instrument jump-threading to update DPValues
jmorse Jun 20, 2023
fd64474
Address test nits
jmorse Nov 23, 2023
83e6dc2
clang-format
jmorse Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions llvm/include/llvm/Transforms/Utils/SSAUpdater.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -123,6 +124,7 @@ 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.
///
Expand All @@ -134,6 +136,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
Expand Down
58 changes: 56 additions & 2 deletions llvm/lib/Transforms/Scalar/JumpThreading.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the AllowEmpty (3rd parameter) situation something to do with use-before-defs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Reached the Cond whose uses we are trying to replace, so there are no
// more uses.
if (&I == Cond)
Expand Down Expand Up @@ -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
Expand All @@ -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())
Expand All @@ -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");
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -2069,6 +2105,8 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
ValueMapping[&*BI] = New;
adaptNoAliasScopes(New, ClonedScopes, Context);

CloneAndRemapDbgInfo(New, &*BI);

if (RetargetDbgValueIfPossible(New))
continue;

Expand All @@ -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;
}

Expand Down Expand Up @@ -2666,13 +2715,18 @@ 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;
}
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)))
Expand Down
27 changes: 24 additions & 3 deletions llvm/lib/Transforms/Utils/SSAUpdater.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -214,16 +220,31 @@ 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)) {
Value *NewVal = GetValueAtEndOfBlock(UserBB);
DbgValue->replaceVariableLocationOp(I, NewVal);
}
else
} else
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());

Expand Down
1 change: 1 addition & 0 deletions llvm/test/Transforms/JumpThreading/redundant-dbg-info.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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:
Expand Down
46 changes: 46 additions & 0 deletions llvm/test/Transforms/JumpThreading/thread-debug-info.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 its
; debug-info should still be preserved.
; Similarly, the call to f1 gets cloned, its 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give the dbg.values different constant values to easily identify which is which in the output/CHECK lines?

; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 1,
; 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 1, 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()
Expand Down Expand Up @@ -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)