Skip to content

[DebugInfo][RemoveDIs] Implement redundant elimination for DPValues #72284

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.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
63 changes: 63 additions & 0 deletions llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,38 @@ bool llvm::MergeBlockSuccessorsIntoGivenBlocks(
/// - Check fully overlapping fragments and not only identical fragments.
/// - Support dbg.declare. dbg.label, and possibly other meta instructions being
/// part of the sequence of consecutive instructions.
static bool DPValuesRemoveRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
SmallVector<DPValue *, 8> ToBeRemoved;
SmallDenseSet<DebugVariable> VariableSet;
for (auto &I : reverse(*BB)) {
for (DPValue &DPV : reverse(I.getDbgValueRange())) {
DebugVariable Key(DPV.getVariable(),
DPV.getExpression(),
DPV.getDebugLoc()->getInlinedAt());
auto R = VariableSet.insert(Key);
// If the same variable fragment is described more than once it is enough
// to keep the last one (i.e. the first found since we for reverse
// iteration).
if (!R.second)
Copy link
Contributor

Choose a reason for hiding this comment

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

// FIXME: Add assignment-tracking support (see removeRedundantDbgInstrsUsingBackwardScan).?

(maybe not worth the note if it's not been done everywhere)

ToBeRemoved.push_back(&DPV);
continue;
}
// Sequence with consecutive dbg.value instrs ended. Clear the map to
// restart identifying redundant instructions if case we find another
// dbg.value sequence.
VariableSet.clear();
}

for (auto &DPV : ToBeRemoved)
DPV->eraseFromParent();

return !ToBeRemoved.empty();
}

static bool removeRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
if (BB->IsNewDbgInfoFormat)
return DPValuesRemoveRedundantDbgInstrsUsingBackwardScan(BB);

SmallVector<DbgValueInst *, 8> ToBeRemoved;
SmallDenseSet<DebugVariable> VariableSet;
for (auto &I : reverse(*BB)) {
Expand Down Expand Up @@ -440,7 +471,39 @@ static bool removeRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
///
/// Possible improvements:
/// - Keep track of non-overlapping fragments.
static bool DPValuesRemoveRedundantDbgInstrsUsingForwardScan(BasicBlock *BB) {
SmallVector<DPValue *, 8> ToBeRemoved;
DenseMap<DebugVariable, std::pair<SmallVector<Value *, 4>, DIExpression *>>
VariableMap;
for (auto &I : *BB) {
for (DPValue &DPV : I.getDbgValueRange()) {
DebugVariable Key(DPV.getVariable(),
std::nullopt,
DPV.getDebugLoc()->getInlinedAt());
Comment on lines +480 to +482
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to keep the code as similar as possible to the dbg.value version then ignore this, but we could use DebugVariableAggregate here now instead of DebugVariable.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can see, the "Aggregate" version only happens down dbg.assign code paths? (Which I'm ignoring for the now)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nothing to do with dbg.assigns - it's just a specialised DebugVariable without a FragmentInfo component, to indicate that it refers to the whole variable. The DebugVariable above is created with a std::nullopt FragmentInfo unconditionally, which is essentially the same thing as a DebugVariableAggregate. It was more a matter of "should we update this code while we're there?" as the change is NFC. No is still a fine answer, I just thought it should be clarified.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah cool, for some reason my mind removed the "then ignore this" wording and thought it was a direct suggestion. Indeed, IMO keeping identical-ness is more important at this stage.

auto VMI = VariableMap.find(Key);
// Update the map if we found a new value/expression describing the
// variable, or if the variable wasn't mapped already.
SmallVector<Value *, 4> Values(DPV.location_ops());
if (VMI == VariableMap.end() || VMI->second.first != Values ||
VMI->second.second != DPV.getExpression()) {
VariableMap[Key] = {Values, DPV.getExpression()};
continue;
}
// Found an identical mapping. Remember the instruction for later removal.
ToBeRemoved.push_back(&DPV);
}
}

for (auto *DPV : ToBeRemoved)
DPV->eraseFromParent();

return !ToBeRemoved.empty();
}

static bool removeRedundantDbgInstrsUsingForwardScan(BasicBlock *BB) {
if (BB->IsNewDbgInfoFormat)
return DPValuesRemoveRedundantDbgInstrsUsingForwardScan(BB);

SmallVector<DbgValueInst *, 8> ToBeRemoved;
DenseMap<DebugVariable, std::pair<SmallVector<Value *, 4>, DIExpression *>>
VariableMap;
Expand Down
1 change: 1 addition & 0 deletions llvm/test/Transforms/DCE/dbg-value-removal.ll
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -S -passes=redundant-dbg-inst-elim | FileCheck %s
; RUN: opt < %s -S -passes=redundant-dbg-inst-elim --try-experimental-debuginfo-iterators | FileCheck %s

; All dbg.value with location "!dbg !19" are redundant in the input.
; FIXME: We do not handle non-overlapping/overlapping fragments perfectly yet.
Expand Down