This is an archive of the discontinued LLVM Phabricator instance.

ThinLTO/ModuleLinker: add a flag to not always pull-in linkonce when performing importing
ClosedPublic

Authored by mehdi_amini on Apr 14 2016, 12:44 AM.

Details

Summary

The function importer already decided what symbols need to be pulled
in. Also these magically added ones will not be in the export list
for the source module, which can confuse the internalizer for
instance.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to ModuleLinker: do not always pull-in linkonce_odr when performing ThinLTO importing.
mehdi_amini updated this object.
mehdi_amini added reviewers: tejohnson, rafael.
mehdi_amini added a subscriber: llvm-commits.
mehdi_amini added inline comments.Apr 14 2016, 12:46 AM
test/Transforms/FunctionImport/funcimport.ll
84–85

I'm not happy about the extern_weak, but apparently the IRMover does not have any option to control that, which is annoying.
Rafael, any input on that?

Update ThinLTO generator to take this into account

tejohnson added inline comments.Apr 15 2016, 6:16 AM
lib/Linker/LinkModules.cpp
421

Noting down the concern I had expressed on IRC yesterday:

This is only legal if the LinkOnce is being eventually changed to something non-discardable, e.g. with the outstanding patches for libLTO (D18908) and gold (D18674, which applies when launching the backend threads through gold). However, even with those there are currently issues because neither patch handles linkonce that are involved with an alias. Any that will remain linkonce, because they aren't involved with an alias, or aren't covered by one of the two noted patches (e.g. using distributed back ends for ThinLTO), still need to be force imported or there will be failures (because the linkonce is discardable in its original exporting module).

My suggestion for fixing this is:

  1. Cover aliases, either by allowing them to be resolved (via the proposed copy technique) or force import them when we collect the imports/exports lists from the index;

and

  1. Indicate to the module linker when it is safe to ignore linkonce here, either by noting the new linkage in the index (which needs to be passed in here somehow), or use a flag to tell the module linker that it is safe to ignore linkonce here when importing (only for libLTO for now, when #1 above is addressed).

Update: resolve alias correctly now. Use a ModuleLinker flag to control the behavior.

mehdi_amini added inline comments.Apr 16 2016, 1:57 AM
lib/Linker/LinkModules.cpp
421

Yes this is only legal if the client code handles it correctly.
I'd like to remove totally any "magic" in general: it adds coupling and make the design too monolithic/complex. Here the client is passing a list of globals to import, I don't expect the "module linker" to import anything else.

I think we talked a few weeks ago about that: ThinLTO importing should be using only the IRMover, not the Linker.
You were concerned about decision that would be taken in the backend that wouldn't match what the ThinLink decided: this is such a good case. The key point is that you have two different logic for the decision (summary based and non-summary based). This is quite unsafe (and unsound)!

I updated the patch to add a ModuleLinker flag that controls if referenced linkonce are imported or not, and I also improve the handling for aliases.

tejohnson edited edge metadata.Apr 18 2016, 8:40 AM

Still think we have a potential issue, see comments embedded in the odr_resolution test case.

include/llvm/Linker/Linker.h
33

Suggest as clearer:

/// Don't force link referenced linkonce definitions.

(i.e. "linkonce definitions" instead of "definition ... linkonce")

34

s/DontLazyAddLinkone/DontLazyAddLinkonce/ (s/one/once)

lib/LTO/ThinLTOCodeGenerator.cpp
162 ↗(On Diff #53980)

Missing space after 'if'.

163 ↗(On Diff #53980)

It took me some time to convince myself that this was going to be correct in all cases. Specifically, I was concerned about the case where an imported function referenced an aliasee that remained linkonce (wasn't first def for linker), and we ended up trying to create a linkonce declaration (illegal) due to the change in addLazyFor. Then I saw the code in the IRMover that turns such declarations into ExternalWeak (as you imply in your question to Rafael in the test case, it doesn't need to be extern_weak, but the important point is that it isn't left linkonce_odr, and we will have a copy emitted in one of the modules for the linker to resolve it with).

I think it would be helpful to add a comment here about how this works. I.e. something like "If aliasee that remains linkonce is referenced by an imported function, the IRMover will convert the declaration to ExternalWeak, which will be satisfied at link time with the copy that was resolved to WeakODR."

lib/Linker/LinkModules.cpp
421

Yes this is only legal if the client code handles it correctly.
I'd like to remove totally any "magic" in general: it adds coupling and make the design too
monolithic/complex. Here the client is passing a list of globals to import, I don't expect the
"module linker" to import anything else.

I think we talked a few weeks ago about that: ThinLTO importing should be using only the
IRMover, not the Linker.
You were concerned about decision that would be taken in the backend that wouldn't match what
the ThinLink decided: this is such a good case. The key point is that you have two different logic
for the decision (summary based and non-summary based). This is quite unsafe (and unsound)!

It is unsound once the thin link step starts making optimizations based on the importing decisions, which is a good direction to go in. But first we have to get all of the ThinLTO clients to handle all linkonce cases correctly. This patch is a good start, and similar handling can be added to the gold-plugin and distributed backend cases. Eventually if the resolved linkage is updated in the index in all cases, we can add some assertion checking to ensure that we don't end up importing a reference to something that remains linkonce but doesn't have its definition in the GlobalsToLink set.

test/ThinLTO/X86/odr_resolution.ll
13 ↗(On Diff #53980)

Can you remind me how we prevent the alias from being optimized this way? We have AliasSummary in DefinedGlobals, but only the aliasee are added to the GlobalInvolvedWithAlias set. I don't see where it is preventing the ODR resolution for the alias itself. If we did try to do the alias resolution, we would end up with an available_externally alias in MOD2, which is illegal.

If we do in fact prevent the ODR resolution in this case, then I think there is a different issue. If we have a reference to the alias in an imported routine, with the change to addLazyFor() we would no longer import this linkonce alias as a definition, and the IRMover will convert the imported declaration to ExternalWeak. But with all the copies of the defintion remaining linkonce, we aren't guaranteed to emit a copy anywhere, they could all be eliminated after inlining in their original modules, and we have an undef at link time.

mehdi_amini added inline comments.Apr 18 2016, 5:07 PM
test/ThinLTO/X86/odr_resolution.ll
13 ↗(On Diff #53980)

Yes, it is done late in fixupODR() does not iterate over aliases in ThinLTOCodeGenerator, and yeah it is bad...
I'll write a test case with a linkonce_odr alias

mehdi_amini edited edge metadata.

Work in progress: start resolving alias ODR. Add a test that should provide better coverage.

Still need to update the function importer to remove the "magic" there, and think a bit more about every possible case.

rafael added inline comments.Apr 19 2016, 6:00 AM
test/ThinLTO/X86/alias_resolution.ll
49 ↗(On Diff #54170)

Is this valid? I would expect extern_weak to be only for declarations and aliases are always definitions.

Should be in better shape now.

mehdi_amini marked 5 inline comments as done.

Reintroduce the linker flag I remove inadvertently in the previous patch

rafael added inline comments.Apr 19 2016, 1:48 PM
test/ThinLTO/X86/alias_resolution.ll
57 ↗(On Diff #54237)

Are we still producing an extern_weak alias? This should be failing the verifier...

tejohnson added inline comments.Apr 19 2016, 5:13 PM
lib/LTO/ThinLTOCodeGenerator.cpp
230 ↗(On Diff #54237)

Doesn't this mean we will end up with available_externally aliases, which would be illegal? I.e. alias was linkonce or weak and is not the first definition for linker, so NewLinkage is available_externally.

lib/Transforms/IPO/FunctionImport.cpp
178–181

The fix to move this check here instead of being later in importFunctions() seems independent from the rest of this patch - can/should it be committed separately?

tools/llvm-link/llvm-link.cpp
280

Doesn't the ODR resolution need to be done to set this flag? I don't see it being done in llvm-link.

mehdi_amini added inline comments.Apr 19 2016, 5:25 PM
lib/LTO/ThinLTOCodeGenerator.cpp
160 ↗(On Diff #54234)

(This code was removed)

tools/llvm-link/llvm-link.cpp
280

I'm not sure what you'd see to resolve here, this is the mode where llvm-link is instructed on the command line to import a specific list of functions.

tejohnson added inline comments.Apr 19 2016, 5:31 PM
tools/llvm-link/llvm-link.cpp
280

Ah yes, nevermind. Just the other two comments/questions then.

mehdi_amini added inline comments.Apr 19 2016, 6:10 PM
lib/Transforms/IPO/FunctionImport.cpp
178–181

r266845

Update, after splitting as much as possible in D19308.

This now only adds the flag to the Linker.

mehdi_amini retitled this revision from ModuleLinker: do not always pull-in linkonce_odr when performing ThinLTO importing to ThinLTO/ModuleLinker: add a flag to not always pull-in linkonce when performing importing.Apr 19 2016, 10:48 PM
mehdi_amini updated this object.
mehdi_amini marked 8 inline comments as done.

Thanks for splitting this out - it made me see what I missed before about where the new flag is being set. See comments below.

Also, should this be marked as dependent on D19308?

lib/Transforms/IPO/FunctionImport.cpp
425

This should be passed down from the client, not hardwired here. E.g. this resolution is not yet done in gold. E.g. pass down to importFunctions() or to FunctionImporter constructor.

test/Linker/funcimport2.ll
6

Looks like this change should be part of D19308 instead?

test/ThinLTO/X86/Inputs/alias_resolution.ll
5 ↗(On Diff #54319)

Looks like this was accidentally left in this patch (test/ThinLTO/X86/alias_resolution.ll is gone). Should this test now be part of D19308?

mehdi_amini added inline comments.
lib/Transforms/IPO/FunctionImport.cpp
425

Will update

test/Linker/funcimport2.ll
6

I don't think so: it is testing that we don't import foo (we're still seeing the original definition). So it is not about the resolution in D19308

test/ThinLTO/X86/Inputs/alias_resolution.ll
5 ↗(On Diff #54319)

Yes,

tejohnson added inline comments.Apr 20 2016, 8:38 AM
test/Linker/funcimport2.ll
6

Ah, I see. Thought this was testing a linkage change, missed the fact that it is already available_externally in this file.

mehdi_amini added inline comments.Apr 20 2016, 12:04 PM
lib/Transforms/IPO/FunctionImport.cpp
425

I've been trying to add a flag to ComputeCrossModuleImport to force-import referenced discardable function, but it is very intrusive to handle.
I also tried to do it as a post-pass, but this does not work (it really can't).

Passing the flag to the FunctionImporter is wrong: this conflicts with the Import/Export list, so I don't want to go this route.

At this point, the easiest route is to force turn linkonce into weak unconditionally in clients.

tejohnson added inline comments.Apr 20 2016, 12:10 PM
lib/Transforms/IPO/FunctionImport.cpp
425

Passing the flag to the FunctionImporter is wrong: this conflicts with the Import/Export list, so I don't want to go this route.

Why? I thought the very reason for adding the flag was because some clients don't currently do the linkonce resolution (and therefore can't do optimization based on the import/export list in the thin link step).

Otherwise there was no reason to add a flag and change addLazyFor to check that instead of whether we are doing importing.

mehdi_amini updated this object.

Thread the flag to the client of the importer, and default it to true for the functionimportpass.

(also remove the wrong alias test case from this patch)

mehdi_amini added inline comments.Apr 20 2016, 12:59 PM
lib/Transforms/IPO/FunctionImport.cpp
425

I thought the very reason for adding the flag was because some clients don't currently do the linkonce resolution (and therefore can't do optimization based on the import/export list in the thin link step).
Otherwise there was no reason to add a flag and change addLazyFor to check that instead of whether we are doing importing.

That's a good point indeed: I should not add the flag to the ModuleLinker at all... (I am less reluctant to special-case the ModuleLinker, because it is already special-cased all around).

Passing the flag to the FunctionImporter is wrong: this conflicts with the Import/Export list, so I don't want to go this route.

Why?

Here is the 3 components I try to isolate: Linker level resolution, Importing decision, and actual Importing

  1. The linker layer is specific (gold plugin, ThinLTOCodeGenerator, etc.), even though at some point I think we could have a common interface that'd call back into the linker somehow.

This layer resolves weak and linkonce (TODO: this is not communicated to the next layer right now), and indicates what can be internalized and what needs to be preserved *outside of the ThinLTO partition*. libLTO (include/llvm-c/lto.h) should implement the API to pass information from this layer to the next one.

  1. The ThinLTO importing decision layer, which is the *only* place were you decide what is imported/exported from a module
  1. Compute linkage adjustments using information from 1 and 2.
  1. The actual IR moving/update, using information from 1/2/3. I see it as a "thin" FunctionImporter that drives the IRMover using the information from the previous layer.

We're not there yet, but going in this direction at least, and I keep fighting the existing machinery to split out these layer.
The ThinLTOCodeGenerator is somehow piling work-around for these, but I hope to be able to refactor it heavily to extract pieces at some point.

This patch is moving something that happens during "4" towards the upper layers, the flag forces to keep this "feature" in the layer 4 (without good reason actually).

tejohnson accepted this revision.Apr 20 2016, 12:59 PM
tejohnson edited edge metadata.

LGTM thanks

This revision is now accepted and ready to land.Apr 20 2016, 12:59 PM
mehdi_amini edited edge metadata.

Rebase/restore the test

In D19096#406930, @joker.eph wrote:

Rebase/restore the test

I thought test/ThinLTO/X86/alias_resolution.ll was supposed to move to D19308?

Oh I moved the test in the wrong patch...

Re-upload the right patch, remove the wrong test!

In D19096#406940, @joker.eph wrote:

Re-upload the right patch, remove the wrong test!

You might have noticed this already but FYI it is still not the right version (looks like an old version)

Hopefully this is the right one...

In D19096#406958, @joker.eph wrote:

Hopefully this is the right one...

LGTM

mehdi_amini added inline comments.Apr 20 2016, 1:39 PM
test/Transforms/FunctionImport/funcimport.ll
85

This check is broken because of the flag now, needs to add a command line flag probably...

tejohnson added inline comments.Apr 20 2016, 1:42 PM
test/Transforms/FunctionImport/funcimport.ll
85

Or check for this behavior through one of the llvm-lto based tests in test/ThinLTO/X86, not with an opt test here.

Update with a command line flag to allows the test to pass.

Add a check that without the flag, the linkonce_odr is imported as expected

mehdi_amini added inline comments.Apr 20 2016, 1:57 PM
test/Transforms/FunctionImport/funcimport.ll
85

Already implemented the flag in the meantime, are you OK with the current patch?

tejohnson added inline comments.Apr 20 2016, 1:59 PM
test/Transforms/FunctionImport/funcimport.ll
85

Yep, thanks.