[PATCH] D145318: [IRLinker] Fix mapping of declaration metadata

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1066
     if (auto Remangled = Intrinsic::remangleIntrinsicFunction(F)) {
+      UnmappedMetadata.erase(F);
       NewGV->eraseFromParent();
----------------
Looking at remangleIntrinsicFunction, it doesn't appear to copy any metadata from F. I'm not sure if this is because intrinsic function declarations can never have metadata, or it just hasn't been needed thus far. I think it would be safer to add NewGV to the UnmappedMetadata set (if F was in it) in case the latter is true, meaning there is no guarantee we couldn't have any that needs to be mapped in the future.

Also, it would be good to expand your test case to test this case (i.e. I assume we end up with a crash if an intrinsic that was renamed had ended up in the UnmappedMetadata set without this change?).

Actually, in thinking through the case that presumably had an issue without this change (which I'm assuming was an intrinsic that was added to the UnmappedMetadata that did not in fact have any metadata, since I don't see remangleIntrinsicFunction copying any metadata), I'm wondering if we can reduce the amount of unnecessary stuff initially added to UnmappedMetadata, but guarding it with a check of NewGO->hasMetadata() ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145318/new/

https://reviews.llvm.org/D145318