[DebugInfo][RemoveDIs] Extract DPValues in CodeExtractor like dbg.values #73252
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
CodeExtractor shifts dbg.value intrinsics out of the region being extracted and updates them to be appropriate in the extracted function. With new non-intrinsic variable locations, we need to manually do this too, with DPValues. Most of this patch shifts and refactors some utilities in fixupDebugInfoPostExtraction so that we can add a single extra helper lambda that iterates over DPValues and applies update-utilities. We also have to assign the IsNewDbgInfoFormat flag in a bunch of places -- this normally gets set the moment you insert a block into a function (or function into a module), however a few blocks are constructed here before being inserted, thus we have to do some manual setup. Tested via LoopExtractor_alloca.ll, which invokes debugify.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesCodeExtractor shifts dbg.value intrinsics out of the region being extracted and updates them to be appropriate in the extracted function. With new non-intrinsic variable locations, we need to manually do this too, with DPValues. Most of this patch shifts and refactors some utilities in fixupDebugInfoPostExtraction so that we can add a single extra helper lambda that iterates over DPValues and applies update-utilities. We also have to assign the IsNewDbgInfoFormat flag in a bunch of places -- this normally gets set the moment you insert a block into a function (or function into a module), however a few blocks are constructed here before being inserted, thus we have to do some manual setup. Tested via LoopExtractor_alloca.ll, which invokes debugify. Full diff: https://github.com/llvm/llvm-project/pull/73252.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 405753158e9cc2b..b96c18edeb11580 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -768,6 +768,7 @@ void CodeExtractor::severSplitPHINodesOfExits(
NewBB = BasicBlock::Create(ExitBB->getContext(),
ExitBB->getName() + ".split",
ExitBB->getParent(), ExitBB);
+ NewBB->IsNewDbgInfoFormat = ExitBB->IsNewDbgInfoFormat;
SmallVector<BasicBlock *, 4> Preds(predecessors(ExitBB));
for (BasicBlock *PredBB : Preds)
if (Blocks.count(PredBB))
@@ -889,6 +890,7 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs,
Function *newFunction = Function::Create(
funcType, GlobalValue::InternalLinkage, oldFunction->getAddressSpace(),
oldFunction->getName() + "." + SuffixToUse, M);
+ newFunction->IsNewDbgInfoFormat = oldFunction->IsNewDbgInfoFormat;
// Inherit all of the target dependent attributes and white-listed
// target independent attributes.
@@ -1505,10 +1507,14 @@ void CodeExtractor::calculateNewCallTerminatorWeights(
static void eraseDebugIntrinsicsWithNonLocalRefs(Function &F) {
for (Instruction &I : instructions(F)) {
SmallVector<DbgVariableIntrinsic *, 4> DbgUsers;
- findDbgUsers(DbgUsers, &I);
+ SmallVector<DPValue *, 4> DPValues;
+ findDbgUsers(DbgUsers, &I, &DPValues);
for (DbgVariableIntrinsic *DVI : DbgUsers)
if (DVI->getFunction() != &F)
DVI->eraseFromParent();
+ for (DPValue *DPV : DPValues)
+ if (DPV->getFunction() != &F)
+ DPV->eraseFromParent();
}
}
@@ -1544,6 +1550,16 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
/*LineNo=*/0, SPType, /*ScopeLine=*/0, DINode::FlagZero, SPFlags);
NewFunc.setSubprogram(NewSP);
+ auto IsInvalidLocation = [&NewFunc](Value *Location) {
+ // Location is invalid if it isn't a constant or an instruction, or is an
+ // instruction but isn't in the new function.
+ if (!Location ||
+ (!isa<Constant>(Location) && !isa<Instruction>(Location)))
+ return true;
+ Instruction *LocationInst = dyn_cast<Instruction>(Location);
+ return LocationInst && LocationInst->getFunction() != &NewFunc;
+ };
+
// Debug intrinsics in the new function need to be updated in one of two
// ways:
// 1) They need to be deleted, because they describe a value in the old
@@ -1552,8 +1568,41 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
// point to a variable in the wrong scope.
SmallDenseMap<DINode *, DINode *> RemappedMetadata;
SmallVector<Instruction *, 4> DebugIntrinsicsToDelete;
+ SmallVector<DPValue*, 4> DPVsToDelete;
DenseMap<const MDNode *, MDNode *> Cache;
+
+ auto GetUpdatedDIVariable = [&] (DILocalVariable *OldVar) {
+ DINode *&NewVar = RemappedMetadata[OldVar];
+ if (!NewVar) {
+ DILocalScope *NewScope = DILocalScope::cloneScopeForSubprogram(
+ *OldVar->getScope(), *NewSP, Ctx, Cache);
+ NewVar = DIB.createAutoVariable(
+ NewScope, OldVar->getName(), OldVar->getFile(), OldVar->getLine(),
+ OldVar->getType(), /*AlwaysPreserve=*/false, DINode::FlagZero,
+ OldVar->getAlignInBits());
+ }
+ return cast<DILocalVariable>(NewVar);
+ };
+
+ auto UpdateDPValuesOnInst = [&](Instruction &I) -> void {
+ if (!I.hasDbgValues())
+ return;
+ for (auto &DPV : I.getDbgValueRange()) {
+ // Apply the two updates that dbg.values get: invalid operands, and
+ // variable metadata fixup.
+ if (any_of(DPV.location_ops(), IsInvalidLocation)) {
+ DPVsToDelete.push_back(&DPV);
+ continue;
+ }
+ if (!DPV.getDebugLoc().getInlinedAt())
+ DPV.setVariable(GetUpdatedDIVariable(DPV.getVariable()));
+ DPV.setDebugLoc(DebugLoc::replaceInlinedAtSubprogram(DPV.getDebugLoc(), *NewSP, Ctx, Cache));
+ }
+ };
+
for (Instruction &I : instructions(NewFunc)) {
+ UpdateDPValuesOnInst(I);
+
auto *DII = dyn_cast<DbgInfoIntrinsic>(&I);
if (!DII)
continue;
@@ -1575,16 +1624,6 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
continue;
}
- auto IsInvalidLocation = [&NewFunc](Value *Location) {
- // Location is invalid if it isn't a constant or an instruction, or is an
- // instruction but isn't in the new function.
- if (!Location ||
- (!isa<Constant>(Location) && !isa<Instruction>(Location)))
- return true;
- Instruction *LocationInst = dyn_cast<Instruction>(Location);
- return LocationInst && LocationInst->getFunction() != &NewFunc;
- };
-
auto *DVI = cast<DbgVariableIntrinsic>(DII);
// If any of the used locations are invalid, delete the intrinsic.
if (any_of(DVI->location_ops(), IsInvalidLocation)) {
@@ -1599,23 +1638,14 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
}
// If the variable was in the scope of the old function, i.e. it was not
// inlined, point the intrinsic to a fresh variable within the new function.
- if (!DVI->getDebugLoc().getInlinedAt()) {
- DILocalVariable *OldVar = DVI->getVariable();
- DINode *&NewVar = RemappedMetadata[OldVar];
- if (!NewVar) {
- DILocalScope *NewScope = DILocalScope::cloneScopeForSubprogram(
- *OldVar->getScope(), *NewSP, Ctx, Cache);
- NewVar = DIB.createAutoVariable(
- NewScope, OldVar->getName(), OldVar->getFile(), OldVar->getLine(),
- OldVar->getType(), /*AlwaysPreserve=*/false, DINode::FlagZero,
- OldVar->getAlignInBits());
- }
- DVI->setVariable(cast<DILocalVariable>(NewVar));
- }
+ if (!DVI->getDebugLoc().getInlinedAt())
+ DVI->setVariable(GetUpdatedDIVariable(DVI->getVariable()));
}
for (auto *DII : DebugIntrinsicsToDelete)
DII->eraseFromParent();
+ for (auto *DPV : DPVsToDelete)
+ DPV->getMarker()->MarkedInstr->dropOneDbgValue(DPV);
DIB.finalizeSubprogram(NewSP);
// Fix up the scope information attached to the line locations in the new
@@ -1721,11 +1751,14 @@ CodeExtractor::extractCodeRegion(const CodeExtractorAnalysisCache &CEAC,
BasicBlock *codeReplacer = BasicBlock::Create(header->getContext(),
"codeRepl", oldFunction,
header);
+ codeReplacer->IsNewDbgInfoFormat = oldFunction->IsNewDbgInfoFormat;
// The new function needs a root node because other nodes can branch to the
// head of the region, but the entry node of a function cannot have preds.
BasicBlock *newFuncRoot = BasicBlock::Create(header->getContext(),
"newFuncRoot");
+ newFuncRoot->IsNewDbgInfoFormat = oldFunction->IsNewDbgInfoFormat;
+
auto *BranchI = BranchInst::Create(header);
// If the original function has debug info, we have to add a debug location
// to the new branch instruction from the artificial entry block.
diff --git a/llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll b/llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll
index 4a7502781851650..48e7816c79595f1 100644
--- a/llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll
+++ b/llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll
@@ -1,4 +1,5 @@
; RUN: opt -passes=debugify,loop-simplify,loop-extract -S < %s | FileCheck %s
+; RUN: opt -passes=debugify,loop-simplify,loop-extract -S < %s --try-experimental-debuginfo-iterators | FileCheck %s
; This tests 2 cases:
; 1. loop1 should be extracted into a function, without extracting %v1 alloca.
|
You can test this locally with the following command:git-clang-format --diff 1fb91beb173b1c3ca93c6ac6e01aab210369e083 6eb63540bf3eb8c6784e2124bb33cf3f84672e1a -- llvm/lib/Transforms/Utils/CodeExtractor.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 9c1186232e..067a325711 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -1553,8 +1553,7 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
auto IsInvalidLocation = [&NewFunc](Value *Location) {
// Location is invalid if it isn't a constant or an instruction, or is an
// instruction but isn't in the new function.
- if (!Location ||
- (!isa<Constant>(Location) && !isa<Instruction>(Location)))
+ if (!Location || (!isa<Constant>(Location) && !isa<Instruction>(Location)))
return true;
Instruction *LocationInst = dyn_cast<Instruction>(Location);
return LocationInst && LocationInst->getFunction() != &NewFunc;
|
OCHyams
approved these changes
Nov 29, 2023
Comment on lines
1588
to
1589
| if (!I.hasDbgValues()) | ||
| return; |
There was a problem hiding this comment.
Is this check really needed?
There was a problem hiding this comment.
I suppose not -- moderately depends on whether there's a performance benefit. Which can be ignored for the moment.
| if (any_of(DPV.location_ops(), IsInvalidLocation)) { | ||
| DPVsToDelete.push_back(&DPV); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
// FIXME: Support dbg.assign intrinsics.
|
NB: not applying all the clang-format recommendations as it affects lines I'm not editing, and I don't want to pollute git-blame |
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.
CodeExtractor shifts dbg.value intrinsics out of the region being extracted and updates them to be appropriate in the extracted function. With new non-intrinsic variable locations, we need to manually do this too, with DPValues.
Most of this patch shifts and refactors some utilities in fixupDebugInfoPostExtraction so that we can add a single extra helper lambda that iterates over DPValues and applies update-utilities. We also have to assign the IsNewDbgInfoFormat flag in a bunch of places -- this normally gets set the moment you insert a block into a function (or function into a module), however a few blocks are constructed here before being inserted, thus we have to do some manual setup.
Tested via LoopExtractor_alloca.ll, which invokes debugify.