[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