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

Thu Nov 12 14:27:16 PST 2015

On Thu, Nov 12, 2015 at 2:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Thu, Nov 12, 2015 at 2:02 PM, Teresa Johnson <tejohnson at google.com>
> wrote:
>>
>> On Thu, Nov 12, 2015 at 1:45 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > 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)
>>
>> ThinLTO isn't whole program, and what you are describing would need
>> whole program analysis to determine which module will emit the comdat
>> into the object file and guarantee that it will be emitted somewhere
>> so that the other modules can drop it even if referencing the comdat
>> members.
>
>
> Not sure I quite follow - why would this require whole program knowledge?
>
> say you've got 3 files, two being ThinLTO'd, one is being compiled as normal
> and then all 3 objects are linked together
>
> The all start off with a comdat group of linkonce_odr entities (one fo the
> ThinLTO'd ones might've got this naturally or by the ThinLTO process). One
> becomes weak_odr, one becomes available_externally, the other remains
> linkonce_odr.

See my previous response. This will require changes/expansion of the
ThinLTO plugin step that currently just combines the summaries - the
only step that sees all the modules, as the modules are optimized
independently in different backend jobs (which also do the importing).
I.e. there needs to be a global process somewhere that decides which
one becomes weak vs staying linkonce, unless I am missing something.

>
> When linked together we still end up with a definition.
>
> Unless weak_odr + linkonce_odr doesn't collapse safely? (if not, what's the
> point of weak rather than external?)
>
>>
>> Weak linkage does what you mention (keep it even if
>> unreferenced, deduplicate in link), but like I said, ThinLTO doesn't
>> have the visibility to make those kind of changes.
>>
>> >
>> >>
>> >>
>> >> >
>> >> > 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?
>>
>> WeakODR is one thing that changed since the proposal was sent (see
>> Duncan't review and my response). They are now imported as
>> available_externally defs (see comments in ModuleLinker::getLinkage
>> for details).
>>
>> LinkOnceODR imported GVs stay LinkOnceODR. The difference between
>> WeakODR and LinkOnceODR is because the latter is discardable if
>> unused, so there is no guarantee that it will be emitted into the
>> object file in its original module (e.g. if all uses inlined). WeakODR
>> is not discardable if unused, so is guaranteed to be available.
>>
>> >
>> >>
>> >>
>> >> >
>> >> > (& 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
>> >
>> >
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>
>

-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413