[PATCH] D19096: ThinLTO/ModuleLinker: add a flag to not always pull-in linkonce when performing importing

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:424
@@ +423,3 @@
+    // Instruct the linker that the client will take care of linkonce resolution
+    unsigned Flags = Linker::Flags::DontForceLinkLinkonceODR;
+
----------------
tejohnson wrote:
> joker.eph wrote:
> > joker.eph wrote:
> > > tejohnson wrote:
> > > > 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.
> > > Will update
> > 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.
> > 
> > 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.
> 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.

2) The ThinLTO importing decision layer, which is the *only* place were you decide what is imported/exported from a module

3) Compute linkage adjustments using information from 1 and 2.

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



http://reviews.llvm.org/D19096