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

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:182
@@ +181,3 @@
+    if (!IsFirstDefinitionForLinker(*GVInfo, Index, ModulePath))
+      GV.setLinkage(GlobalValue::AvailableExternallyLinkage);
+    break;
----------------
joker.eph wrote:
> 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.
I'm not sure how common or hot non-overridden weak symbols are in practice. But yes, if we knew it was prevailing (as gold does) then I believe this could be done. But you need the linker to tell you it is the prevailing copy (not just select the first one seen as here).


http://reviews.llvm.org/D18346