[PATCH] D14623: [ThinLTO] Comdat importing fixes and related cleanup

Thu Nov 12 13:45:31 PST 2015

On Thu, Nov 12, 2015 at 1:21 PM, Teresa Johnson <tejohnson at google.com>
wrote:

> On Thu, Nov 12, 2015 at 12:56 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >
> >
> > On Thu, Nov 12, 2015 at 11:41 AM, Teresa Johnson via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> tejohnson created this revision.
> >> tejohnson added reviewers: rafael, dexonsmith.
> >> tejohnson added subscribers: llvm-commits, davidxl, joker.eph.
> >>
> >> A number of comdat related fixes, cleanup and test cases.
> >>
> >> I made a number of changes to get importing working when there are
> >> comdats, with lots of new test cases involving comdats of various
> >> selection types, linkage types, with and without aliases.
> >>
> >> The changes ensure we link in the full comdat group containing the
> >> function indicated for importing. Previously I intended to require
> >> that the importer specify the additional comdat group members for
> >> importing on successive importing requests. However, in order to get the
> >> comdat selection type correct it is significantly simpler to import the
> >> full comdat group in a single pass.
> >
> >
> > I thought all imported definitions were marked available_externally - in
> > which case I'm not sure why it would be necessary to bring in the whole
> > comdat group, since the comdat would never be emitted here.
>
> Not always, e.g. comdat containing linkonce values.
>
> Even in the case of those brought in available_externally, my concern
> is when the comdat group also exists in the dest module (the one we
> are importing into), but we want to select the one from the source
> module (containing the imported function) because of the comdat
> selection type. I am concerned about importing part of the comdat
> group then eliminating it, leaving an incomplete group. Presumably the
> linker will select the complete group from the right .o file in the
> final link, but it seems wonky to me.
>

Sorry, I'm not quite following. My familiarity with comdats is perhaps
limited - but mostly for linkonce situations "we put this in multiple
object files, but we really only need one copy of the group in the final
executable".

I'm imagining an intermodule optimization where we promote the group in one
file to "always emitted (not just only emitted if called/used - because
this version will be relied upon by other modules now) but can still be
deduplicated by the linker" (maybe we don't have a linkage type for that?
Would weak suffice? From my reading of the LangRef it sounds like exactly
the right thing) and then marking them in all the other modules we see as
available_externally.

In this way, the definition would be available for optimization, but we
would avoid code generating duplicate entities only to have the linker
deduplicate them later. (but, since we don't have whole program information
- we'd still be able to deduplicate this single ThinLTO definition with any
non-ThinLTO definitions at link time)

>
> >
> > I guess I'm missing something - is there a pointer to when/why this is
> > necessary?
>
> See the ThinLTO symbol linkage RFC here:
> https://groups.google.com/d/msg/llvm-dev/-6H4GLv2BJw/77DBR-VjuHkJ.
> There is a summary table at the end, but there are a lot of details.
> It has changed a little bit based on Duncan's feedback and other
> things found during implementation, but it is mostly accurate. See
> also ModuleLinker::getLinkage() in LinkModules.cpp for the implemented
> mapping.
>

Ah, thanks for the ref.

"WeakODRLinkage symbols cannot be marked AvailableExternallyLinkage,
because if the def is later dropped (by the EliminateAvailableExternally
pass), the new decl is marked ExternalLinkage. For these weak symbols,
however, the correct linkage for the decl is actually ExternalWeakLinkage,
so that they get treated appropriately by the linker."

That helps me understand some of it...

*reads further*

It's not called out, but I imagine there's the same problem with
LinkonceODRLinkage? We can't take a linkonce_odr function, change it to
weak_odr, import it into the other module as available_externally? Because
once we drop the definition, we lose the "weak" attribute and would produce
an incorrect reference?

That seems unfortunate - it's mentioned in the weak_odr case that weak
symbols are rare - linkonce_odr functions are probably /really/ common and
might benefit from this. But it's purely a linker/object size optimization,
so I could imagine it would be a valid optimization to defer until later?

>
> >
> > (& in fact, it might be useful to have this pass make one instance of a
> > comdat group into an external definition (I guess that's probably not
> > possible unless you have true whole program knowledge - maybe there's
> > another linkage type which would produce the comdat group even if all the
> > calls are optimized away) - and mark all the other instances
> > available_externally? even potentially stripping the comdat groups from
> > modules where the call sites are really cold)
>
> I think this is not possible without whole program analysis as you
> mentioned.
>
> Thanks,
> Teresa
>
> >
> >>
> >>
> >> Additionally, if the module linker selects the source version of a
> >> comdat not containing the imported function, if it is referenced
> >> by an imported function we also import all its comdat group member
> >> definitions (via the lazy linker) in the same importing pass.
> >> In order to support this, we are now much more aggressive about lazy
> >> linking global values during importing, which only need to be
> >> linked in (as either declarations or definitions) if referenced by an
> >> imported function.
> >>
> >> Also, because we may now import additional function bodies lazily, it is
> >> no longer possible to determine up front whether an alias's aliasee is
> >> going to be imported as a definition or not when we copy its prototype.
> >> Therefore, we now convert aliases into declarations after all lazy
> >> linking is complete, if the aliasee definition was not imported.
> >>
> >> Finally, since comdat members may not be declarations (including
> >> available_externally), after linking is complete those that were
> >> imported as linker declarations will be removed from comdats.
> >>
> >> http://reviews.llvm.org/D14623
> >>
> >> Files:
> >>   lib/Linker/LinkModules.cpp
> >>   test/Linker/Inputs/funcimport_comdat.ll
> >>   test/Linker/Inputs/funcimport_comdat2.ll
> >>   test/Linker/Inputs/funcimport_comdat3.ll
> >>   test/Linker/funcimport.ll
> >>   test/Linker/funcimport_comdat.ll
> >>   test/Linker/funcimport_comdat2.ll
> >>   test/Linker/funcimport_comdat3.ll
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> >
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151112/31f5597f/attachment.html>