[PATCH] D113812: [Cloning] Clone metadata on function declarations
Duncan P. N. Exon Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 1 15:31:06 PST 2021
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
In D113812#3132357 <https://reviews.llvm.org/D113812#3132357>, @Meinersbur wrote:
> In D113812#3129072 <https://reviews.llvm.org/D113812#3129072>, @dexonsmith wrote:
>
>> Huh... when did metadata get added to function declarations?
>
> Since D21052 <https://reviews.llvm.org/D21052>
Thanks, that rings a bell now.
In D113812#3161886 <https://reviews.llvm.org/D113812#3161886>, @aeubanks wrote:
> ping
LGTM! I left a rambling comment inline but I'm not sure there's anything to be done with it.
================
Comment at: llvm/lib/Transforms/Utils/CloneModule.cpp:148-151
+ SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
+ I.getAllMetadata(MDs);
+ for (auto MD : MDs)
+ F->addMetadata(MD.first, *MapMetadata(MD.second, VMap));
----------------
I was initially worried about this skipping the DebugInfoFinder logic mentioned in this comment from CloneFunctionInto:
```
lang=c++
// Duplicate the metadata that is attached to the cloned function.
// Subprograms/CUs/types that were already mapped to themselves won't be
// duplicated.
SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
OldFunc->getAllMetadata(MDs);
for (auto MD : MDs) {
NewFunc->addMetadata(MD.first, *MapMetadata(MD.second, VMap, RemapFlag,
TypeMapper, Materializer));
}
```
Looking at that logic closely, I think it's okay to skip. If the new location is a different module, the DebugInfoFinder is only used for walking the instructions (of which a declaration has none). Only if it's in the same module does it walk the function's attachments.
I thought about trying to put that logic in a function that would be reused here (but be a no-op) in case the logic changes over time. But given that this is a new module, I think it's really unlikely to be useful and would be distracting.
I wonder if there's a comment that should be left somewhere, but I'm not sure what it would say. Maybe it's just obvious that for a new module you don't want special logic like that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113812/new/
https://reviews.llvm.org/D113812
More information about the llvm-commits
mailing list