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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.

lgtm



================
Comment at: llvm/lib/Linker/IRMover.cpp:1066
     if (auto Remangled = Intrinsic::remangleIntrinsicFunction(F)) {
+      UnmappedMetadata.erase(F);
       NewGV->eraseFromParent();
----------------
critson wrote:
> tejohnson wrote:
> > tejohnson wrote:
> > > 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() ?
> > > 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() ?
> > 
> > And if you do that, it should avoid the need to remove F from the UnmappedMetadata set here (under the assumption that we don't currently have intrinsic functions with any metadata at this point). So you can simply replace this line with an assert that F is not in UnmappedMetadata, with a comment noting that remangleIntrinsicFunction does not copy any metadata over, so we assume that F could not have metadata to start with. If the assert is ever hit, then not only does this code need to change but remangleIntrinsicFunction needs to be updated as well.
> This seems like a good solution to me.
> I have not extended the test cases as existing linker tests cover this with the assertion added.
> (Assertion catches what was only being found by MSAN tests.)
Great, an existing test hitting this is good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145318