Page MenuHomePhabricator

[ThinLTO] Import symver directives for imported symbols (PR48214)
ClosedPublic

Authored by hans on Nov 30 2020, 10:56 AM.

Details

Summary

When importing symbols from another module, also import any corresponding symver directives.

(I'm new to this part of the code, so please give me pointers on how I can turn this into something that's fit for landing.)

Diff Detail

Event Timeline

hans created this revision.Nov 30 2020, 10:56 AM
hans requested review of this revision.Nov 30 2020, 10:56 AM

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

llvm/lib/Linker/IRMover.cpp
712

This function gets called once per global value being linked in, so it would redo the below code multiple times.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
490 ↗(On Diff #308425)

leftover debugging output?

hans marked 2 inline comments as done.Dec 1 2020, 7:36 AM

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

Yes, that seems easier. Thanks!

llvm/lib/Linker/IRMover.cpp
712

Yes, to make this work I figured we'd pull the symver metadata into a data structure that could be queried quickly here. But this approach no longer applies anyway.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
490 ↗(On Diff #308425)

Oops, yes. But these changes are no longer necessary anyway.

hans updated this revision to Diff 308655.Dec 1 2020, 7:39 AM
hans marked 2 inline comments as done.
hans edited the summary of this revision. (Show Details)

Pulling in the symver directives in IRLinker::run() instead. (I didn't do a separate function since it's just a few lines, but happy to do that if you think it would be better.)

Please take another look.

hans updated this revision to Diff 308657.Dec 1 2020, 7:43 AM

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

I think that's fine to have that dependence.

Largely lgtm but suggestion for refactor and test below.

llvm/lib/Linker/IRMover.cpp
1473

I think it makes sense to handle this earlier where we handle inline asm when !IsPerformingImport, since they logically go together (i.e. for regular LTO merge them, for ThinLTO just pull in the symver). Maybe refactor out the two sets of handling into a single helper method that deals with inline asm.

llvm/test/ThinLTO/X86/import-symver.ll
9

Maybe show a second case where if we don't import bar we also don't import its symver? You could add a second case where we disable importing (add -import-instr-limit=0 to an invocation of llvm-lto -thinlto-action=import) and check the results.

hans updated this revision to Diff 308701.Dec 1 2020, 10:18 AM
hans marked an inline comment as done.

Adding test case where the symver doesn't get imported.

llvm/lib/Linker/IRMover.cpp
1473

I looked at that first, but at that point the actual linking/importing into DstM hasn't happened yet (the worklist processing comes a few lines later), and so checking if symbols exist in DstM wouldn't work.

I suppose we could move the !IsPerformingImport inline asm handling down here though. I don't think there'd be any downside to doing that after worklist processing rather than before. Would that be better?

llvm/test/ThinLTO/X86/import-symver.ll
9

Done.

tejohnson added inline comments.Dec 1 2020, 11:22 AM
llvm/lib/Linker/IRMover.cpp
1473

Ah yeah I see the issue. I don't think there would be any issue with moving that earlier handling down here though, so why don't you do that so all the inline asm handling is together.

hans updated this revision to Diff 308725.Dec 1 2020, 11:46 AM

Moving the "Append the module inline asm string" code down, and simplifying it in the process.

hans marked an inline comment as done.Dec 1 2020, 11:47 AM
hans added inline comments.
llvm/lib/Linker/IRMover.cpp
1473

Done.

llvm/test/Linker/link-arm-and-thumb-module-inline-asm.ll
16 ↗(On Diff #308725)

The previous code would output an empty module asm "" line here. My simplification removes that, and also makes the test expectations a bit more explicit.

tejohnson accepted this revision.Dec 1 2020, 12:44 PM

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

This revision is now accepted and ready to land.Dec 1 2020, 12:44 PM
hans added a comment.Dec 2 2020, 5:57 AM

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

Landed that separately as 45d8a7843253ec68367c26114a2aa6bff2a7a4bb