This is an archive of the discontinued LLVM Phabricator instance.

[IRLinker] Fix mapping of declaration metadata
ClosedPublic

Authored by critson on Mar 5 2023, 12:21 AM.

Details

Summary

Ensure metadata for declarations copied during materialization
is properly mapped if declarations do not become definitions.

Diff Detail

Event Timeline

critson created this revision.Mar 5 2023, 12:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2023, 12:21 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
critson requested review of this revision.Mar 5 2023, 12:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2023, 12:21 AM

Is this a case where we didn't in the past have metadata attachments on declarations requiring mapping, and now we do? Also, the example uses a "!types" metadata - I'm familiar with !type metadata on definitions, but haven't seen !types before - is this new metadata? I can't find any uses of it in the test suite or any documentation.

llvm/lib/Linker/IRMover.cpp
1142

GlobalVariable are also GlobalObjects, should they get the same treatment in linkGlobalVariable?

But is this removal needed at all, since the loop over UnmappedMetadata later on ignores definitions?

critson marked an inline comment as done.Mar 5 2023, 6:43 PM

Is this a case where we didn't in the past have metadata attachments on declarations requiring mapping, and now we do? Also, the example uses a "!types" metadata - I'm familiar with !type metadata on definitions, but haven't seen !types before - is this new metadata? I can't find any uses of it in the test suite or any documentation.

!types is custom metadata we are investigating to handle DXIL bitcode.
An example of its generation can be seen in the tests for D127728.

llvm/lib/Linker/IRMover.cpp
1142

I think removing all the erase operations is probably sensible.

critson updated this revision to Diff 502528.Mar 5 2023, 11:16 PM
critson marked an inline comment as done.
  • Remove unnecessary erase calls, simplifying implementation
  • Extend comment
tejohnson accepted this revision.Mar 6 2023, 10:12 AM

Thanks for the context. lgtm with a comment suggestion.

llvm/lib/Linker/IRMover.cpp
412

Add doxygen comment.

This revision is now accepted and ready to land.Mar 6 2023, 10:12 AM
critson marked an inline comment as done.Mar 7 2023, 11:17 PM
This revision was landed with ongoing or failed builds.Mar 7 2023, 11:17 PM
This revision was automatically updated to reflect the committed changes.
critson updated this revision to Diff 503651.Mar 9 2023, 12:02 AM
  • Add missing erasure of functions which are erased through renaming.

can you re-upload with context?

critson reopened this revision.Mar 9 2023, 11:01 PM
This revision is now accepted and ready to land.Mar 9 2023, 11:01 PM
critson updated this revision to Diff 504032.Mar 9 2023, 11:01 PM
  • Add missing context

can you re-upload with context?

Sorry, context should be fixed now.
The only change over the accepted revision is IRMover.cpp:1066.

tejohnson added inline comments.Mar 12 2023, 11:44 AM
llvm/lib/Linker/IRMover.cpp
1064

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() ?

tejohnson added inline comments.Mar 12 2023, 11:51 AM
llvm/lib/Linker/IRMover.cpp
1064

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.

critson updated this revision to Diff 504490.Mar 12 2023, 6:22 PM
  • Exclude global objects without metadata from unmapped set
  • Add assertion to cover the case of intrinsics with metadata
critson marked 2 inline comments as done.Mar 12 2023, 6:25 PM
critson added inline comments.
llvm/lib/Linker/IRMover.cpp
1064

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.)

tejohnson accepted this revision.Mar 13 2023, 7:32 AM

lgtm

llvm/lib/Linker/IRMover.cpp
1064

Great, an existing test hitting this is good.

This revision was automatically updated to reflect the committed changes.
critson marked an inline comment as done.