-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[Coro] Amortize debug info processing cost in CoroSplit #109032
base: main
Are you sure you want to change the base?
Changes from 3 commits
a987688
3cb662c
7b6c39f
a611c5e
06c10b1
fad2d5e
53d8285
b268200
a6cba59
0eefa6a
d7f598c
e14a46f
d1479b9
4c76540
f2ec90a
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 |
---|---|---|
|
@@ -86,28 +86,14 @@ BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap, | |
return NewBB; | ||
} | ||
|
||
// Clone OldFunc into NewFunc, transforming the old arguments into references to | ||
// VMap values. | ||
// | ||
void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | ||
ValueToValueMapTy &VMap, | ||
CloneFunctionChangeType Changes, | ||
SmallVectorImpl<ReturnInst *> &Returns, | ||
const char *NameSuffix, ClonedCodeInfo *CodeInfo, | ||
ValueMapTypeRemapper *TypeMapper, | ||
ValueMaterializer *Materializer) { | ||
NewFunc->setIsNewDbgInfoFormat(OldFunc->IsNewDbgInfoFormat); | ||
assert(NameSuffix && "NameSuffix cannot be null!"); | ||
|
||
#ifndef NDEBUG | ||
for (const Argument &I : OldFunc->args()) | ||
assert(VMap.count(&I) && "No mapping from source argument specified!"); | ||
#endif | ||
|
||
bool ModuleLevelChanges = Changes > CloneFunctionChangeType::LocalChangesOnly; | ||
|
||
// Copy all attributes other than those stored in the AttributeList. We need | ||
// to remap the parameter indices of the AttributeList. | ||
// Copy all attributes other than those stored in the AttributeList. We need | ||
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. I'm not sure this comment is making a lot of sense as a function-level comment. To be honest even the original comment was confusing: which AttributeList? If you know the answer to this, could you take the chance provided by this NFC commit to improve the comment? 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. This second commit could also use a brief commit message describing why this change is useful 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. I think the comment refers to the |
||
// to remap the parameter indices of the AttributeList. | ||
void llvm::CloneFunctionAttributesInto(Function *NewFunc, | ||
const Function *OldFunc, | ||
ValueToValueMapTy &VMap, | ||
bool ModuleLevelChanges, | ||
ValueMapTypeRemapper *TypeMapper, | ||
ValueMaterializer *Materializer) { | ||
AttributeList NewAttrs = NewFunc->getAttributes(); | ||
NewFunc->copyAttributesFrom(OldFunc); | ||
NewFunc->setAttributes(NewAttrs); | ||
|
@@ -147,6 +133,52 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | |
NewFunc->setAttributes( | ||
AttributeList::get(NewFunc->getContext(), OldAttrs.getFnAttrs(), | ||
OldAttrs.getRetAttrs(), NewArgAttrs)); | ||
} | ||
|
||
DISubprogram *llvm::ProcessSubprogramAttachment(const Function &F, | ||
CloneFunctionChangeType Changes, | ||
DebugInfoFinder &DIFinder) { | ||
DISubprogram *SPClonedWithinModule = nullptr; | ||
if (Changes < CloneFunctionChangeType::DifferentModule) { | ||
SPClonedWithinModule = F.getSubprogram(); | ||
} | ||
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. LLVM's coding guidelines prescribe not using |
||
if (SPClonedWithinModule) | ||
DIFinder.processSubprogram(SPClonedWithinModule); | ||
|
||
const Module *M = F.getParent(); | ||
if (Changes != CloneFunctionChangeType::ClonedModule && M) { | ||
// Inspect instructions to process e.g. DILexicalBlocks of inlined functions | ||
for (const auto &BB : F) { | ||
for (const auto &I : BB) { | ||
DIFinder.processInstruction(*M, I); | ||
} | ||
} | ||
} | ||
|
||
return SPClonedWithinModule; | ||
} | ||
|
||
// Clone OldFunc into NewFunc, transforming the old arguments into references to | ||
// VMap values. | ||
void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | ||
ValueToValueMapTy &VMap, | ||
CloneFunctionChangeType Changes, | ||
SmallVectorImpl<ReturnInst *> &Returns, | ||
const char *NameSuffix, ClonedCodeInfo *CodeInfo, | ||
ValueMapTypeRemapper *TypeMapper, | ||
ValueMaterializer *Materializer) { | ||
NewFunc->setIsNewDbgInfoFormat(OldFunc->IsNewDbgInfoFormat); | ||
assert(NameSuffix && "NameSuffix cannot be null!"); | ||
|
||
#ifndef NDEBUG | ||
for (const Argument &I : OldFunc->args()) | ||
assert(VMap.count(&I) && "No mapping from source argument specified!"); | ||
#endif | ||
|
||
bool ModuleLevelChanges = Changes > CloneFunctionChangeType::LocalChangesOnly; | ||
|
||
CloneFunctionAttributesInto(NewFunc, OldFunc, VMap, ModuleLevelChanges, | ||
TypeMapper, Materializer); | ||
|
||
// Everything else beyond this point deals with function instructions, | ||
// so if we are dealing with a function declaration, we're done. | ||
|
@@ -158,23 +190,19 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | |
// duplicate instructions and then freeze them in the MD map. We also record | ||
// information about dbg.value and dbg.declare to avoid duplicating the | ||
// types. | ||
std::optional<DebugInfoFinder> DIFinder; | ||
DebugInfoFinder DIFinder; | ||
|
||
// Track the subprogram attachment that needs to be cloned to fine-tune the | ||
// mapping within the same module. | ||
DISubprogram *SPClonedWithinModule = nullptr; | ||
if (Changes < CloneFunctionChangeType::DifferentModule) { | ||
// Need to find subprograms, types, and compile units. | ||
|
||
assert((NewFunc->getParent() == nullptr || | ||
NewFunc->getParent() == OldFunc->getParent()) && | ||
"Expected NewFunc to have the same parent, or no parent"); | ||
|
||
// Need to find subprograms, types, and compile units. | ||
DIFinder.emplace(); | ||
|
||
SPClonedWithinModule = OldFunc->getSubprogram(); | ||
if (SPClonedWithinModule) | ||
DIFinder->processSubprogram(SPClonedWithinModule); | ||
} else { | ||
// Need to find all the compile units. | ||
|
||
assert((NewFunc->getParent() == nullptr || | ||
NewFunc->getParent() != OldFunc->getParent()) && | ||
"Expected NewFunc to have different parents, or no parent"); | ||
|
@@ -183,19 +211,20 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | |
assert(NewFunc->getParent() && | ||
"Need parent of new function to maintain debug info invariants"); | ||
|
||
// Need to find all the compile units. | ||
DIFinder.emplace(); | ||
} | ||
} | ||
|
||
DISubprogram *SPClonedWithinModule = | ||
ProcessSubprogramAttachment(*OldFunc, Changes, DIFinder); | ||
|
||
// Loop over all of the basic blocks in the function, cloning them as | ||
// appropriate. Note that we save BE this way in order to handle cloning of | ||
// recursive functions into themselves. | ||
for (const BasicBlock &BB : *OldFunc) { | ||
|
||
// Create a new basic block and copy instructions into it! | ||
BasicBlock *CBB = CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo, | ||
DIFinder ? &*DIFinder : nullptr); | ||
BasicBlock *CBB = | ||
CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo, nullptr); | ||
|
||
// Add basic block mapping. | ||
VMap[&BB] = CBB; | ||
|
@@ -218,7 +247,7 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | |
} | ||
|
||
if (Changes < CloneFunctionChangeType::DifferentModule && | ||
DIFinder->subprogram_count() > 0) { | ||
DIFinder.subprogram_count() > 0) { | ||
// Turn on module-level changes, since we need to clone (some of) the | ||
// debug info metadata. | ||
// | ||
|
@@ -233,24 +262,24 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | |
|
||
// Avoid cloning types, compile units, and (other) subprograms. | ||
SmallPtrSet<const DISubprogram *, 16> MappedToSelfSPs; | ||
for (DISubprogram *ISP : DIFinder->subprograms()) { | ||
for (DISubprogram *ISP : DIFinder.subprograms()) { | ||
if (ISP != SPClonedWithinModule) { | ||
mapToSelfIfNew(ISP); | ||
MappedToSelfSPs.insert(ISP); | ||
} | ||
} | ||
|
||
// If a subprogram isn't going to be cloned skip its lexical blocks as well. | ||
for (DIScope *S : DIFinder->scopes()) { | ||
for (DIScope *S : DIFinder.scopes()) { | ||
auto *LScope = dyn_cast<DILocalScope>(S); | ||
if (LScope && MappedToSelfSPs.count(LScope->getSubprogram())) | ||
mapToSelfIfNew(S); | ||
} | ||
|
||
for (DICompileUnit *CU : DIFinder->compile_units()) | ||
for (DICompileUnit *CU : DIFinder.compile_units()) | ||
mapToSelfIfNew(CU); | ||
|
||
for (DIType *Type : DIFinder->types()) | ||
for (DIType *Type : DIFinder.types()) | ||
mapToSelfIfNew(Type); | ||
} else { | ||
assert(!SPClonedWithinModule && | ||
|
@@ -304,7 +333,7 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, | |
SmallPtrSet<const void *, 8> Visited; | ||
for (auto *Operand : NMD->operands()) | ||
Visited.insert(Operand); | ||
for (auto *Unit : DIFinder->compile_units()) { | ||
for (auto *Unit : DIFinder.compile_units()) { | ||
MDNode *MappedUnit = | ||
MapMetadata(Unit, VMap, RF_None, TypeMapper, Materializer); | ||
if (Visited.insert(MappedUnit).second) | ||
|
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.
Could you elaborate on "Process"? This is more about collecting debug info, right?
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.
That's right. This function hydrates the passed DebugInfoFinder by visiting relevant components of the function's subprogram attachment. I'll update the name/comment when we get to extracting that commit in a separate PR.
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.
Sounds good, now that the other two PRs are open & approved, let's proceed with this commit!
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.
Great, yes! @felipepiovezan a silly question -- now that those PRs are approved, how/when do they get merged? Stacking unmerged PRs on top of each other doesn't have the best UX in Github.
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.
So I think the best option here is to merge the two PRs that are approved (#112976 and #112948) . Both of those stand on their own and don't depend on each other, so it's fine to merge in w/e order, right?
And then you can grab the third commit (or more, if the subsequent commits are also independent of each other) of this PR and open a new PR (or multiple PRs, if you did the () before), so it will look fine on top of
main
. My understanding is that the fourth commit depends on the third though.Regarding what to do about this umbrella PR in the mean time, you have two options: leave this as is (with the understanding that it is not meant to be merged, merely serve as a reference for all the work in the proposal), or rebase on top of main, which will get rid of the two-already merged commits and force push to your branch
artempyanykh:fast-coro-upstream
, which will mean this PR serves a reference of the work that's left to be merged, also with the understand that we're not merging it.With all that stack, we'd never have to stack unmerged PRs on top of each other, does that make sense?
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.
By the way, do you have commit access?
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.
That's right, yes.
After the first 2 are merged, extracting the third one in a separate PR is straightforward. With the rest of the stack there's quite a bit of sequencing, so we may need to rinse and repeat.
+1 to this. I was planning to keep rebasing this stack anyway to make sure things keep working end-to-end while the chunks are being extracted and merged.
No, not sure what the process for this is. Why?
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.
Sorry for the delay, the LLVM dev conference happened last week and we were mostly out.
Just wondering if you needed help merging the open PRs. I've merged the first two for you.
The process for getting commit access is described here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
If you don't want to do that, you can always let the reviewers know you need someone to press the merge button for you