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.
Details
Diff Detail
Event Timeline
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. |
lib/Linker/LinkModules.cpp | ||
---|---|---|
435 | 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:
and
|
Update: resolve alias correctly now. Use a ModuleLinker flag to control the behavior.
lib/Linker/LinkModules.cpp | ||
---|---|---|
435 | Yes this is only legal if the client code handles it correctly. I think we talked a few weeks ago about that: ThinLTO importing should be using only the IRMover, not the Linker. 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. |
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 | ||
159 | Missing space after 'if'. | |
160 | 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 | ||
435 |
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 | 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. |
test/ThinLTO/X86/odr_resolution.ll | ||
---|---|---|
13 | Yes, it is done late in fixupODR() does not iterate over aliases in ThinLTOCodeGenerator, and yeah it is bad... |
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.
test/ThinLTO/X86/alias_resolution.ll | ||
---|---|---|
50 | Is this valid? I would expect extern_weak to be only for declarations and aliases are always definitions. |
test/ThinLTO/X86/alias_resolution.ll | ||
---|---|---|
57 | Are we still producing an extern_weak alias? This should be failing the verifier... |
lib/LTO/ThinLTOCodeGenerator.cpp | ||
---|---|---|
230 | 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 | ||
169 | 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. |
tools/llvm-link/llvm-link.cpp | ||
---|---|---|
280 | Ah yes, nevermind. Just the other two comments/questions then. |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
169 | r266845 |
Update, after splitting as much as possible in D19308.
This now only adds the flag to the Linker.
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 | ||
---|---|---|
421 | 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 | ||
6 | 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? |
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. |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
421 | I've been trying to add a flag to ComputeCrossModuleImport to force-import referenced discardable function, but it is very intrusive to handle. 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. |
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
421 |
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. |
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)
lib/Transforms/IPO/FunctionImport.cpp | ||
---|---|---|
421 |
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).
Here is the 3 components I try to isolate: Linker level resolution, Importing decision, and actual Importing
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.
We're not there yet, but going in this direction at least, and I keep fighting the existing machinery to split out these layer. 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). |
You might have noticed this already but FYI it is still not the right version (looks like an old version)
test/Transforms/FunctionImport/funcimport.ll | ||
---|---|---|
85 | This check is broken because of the flag now, needs to add a command line flag probably... |
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. |
test/Transforms/FunctionImport/funcimport.ll | ||
---|---|---|
85 | Already implemented the flag in the meantime, are you OK with the current patch? |
test/Transforms/FunctionImport/funcimport.ll | ||
---|---|---|
85 | Yep, thanks. |
Suggest as clearer:
/// Don't force link referenced linkonce definitions.
(i.e. "linkonce definitions" instead of "definition ... linkonce")