-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
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)) { | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
There was a problem hiding this comment.
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)