[PATCH] D18346: ThinLTO: special handling for LinkOnce functions

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:182
@@ +181,3 @@
+    if (!IsFirstDefinitionForLinker(*GVInfo, Index, ModulePath))
+      GV.setLinkage(GlobalValue::AvailableExternallyLinkage);
+    break;
----------------
tejohnson wrote:
> joker.eph wrote:
> > joker.eph wrote:
> > > tejohnson wrote:
> > > > Thinking more about this - I don't think it is correct for WeakAny or LinkOnceAny to be converted to AvailableExternally. This could change the program result. The reason is that the selected copy is allowed to have different behavior than the unselected version. This is why WeakAny and LinkOnceAny return true from GlobalValue::mayBeOverridden(), which prevents optimizations such as inlining and therefore ensures that the linker-selected copy is executed. Changing their linkage to AvailableExternally enables the unselected versions to be inlined, and therefore the selected body is not executed at runtime, which can result in different program behavior.
> > > Yeah that's a very good catch.
> > > 
> > > It lead me to a more interesting point: if we know which copy the linker will select, we should turn it into a strong definition and *eliminate* all the other. This way the strong one will be imported and could be inlined where it usually can't. I imagine gold is already doing that in LTO.
> > Maybe we can't turn a weak into a strong, because the dynamic loader can override it with another symbol in another library?
> In LTO mode gold will simply internalize the prevailing copy and the rest get dropped during merging.
> 
> What you can do in the non-ODR case is to keep (via weak linkage as you are here) the first copy, and drop the definitions for the other copies (instead of converting them to avail externally). That way the remaining weak copy can still be overridden if necessary by a strong def in another library.
Makes me think: Is there any interest in importing a weak def somewhere else?
If it is the prevailing copy and we can turn it to a strong def, then yes, otherwise there is not much interest.


http://reviews.llvm.org/D18346