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

[Teresa Johnson via llvm-commits]

On Thu, Nov 12, 2015 at 2:32 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Thu, Nov 12, 2015 at 2:27 PM, Teresa Johnson <tejohnson at google.com>
> wrote:
>>
>> 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).
>
>
> Oh, that's something I hadn't noticed/realized/understood perhaps.
>
> OK, so the ThinLTO step doesn't even have access to the modules? Just the
> summaries?

I assume you mean the ThinLTO plugin step. It does have access to the
modules (the summaries are a couple blocks in the module bitcode). But
in order to make ThinLTO super scalable, it does as little work as
possible. We only parse the summaries out and skip everything else.

>
> Then the backend jobs (these are the separate actual optimization pipeline
> stage that produces object files, yes?)

Yep

> have access to all the modules and
> the ThinLTO produced updated import summary info?

Yes. It looks at the summary to make a decision whether to import and
to locate the module to import from, it then only reads the minimal
amount of bitcode from that module to 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
>
>



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