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

Thu Nov 12 14:29:51 PST 2015

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

> On Thu, Nov 12, 2015 at 2:06 PM, David Blaikie via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> >
> > On Thu, Nov 12, 2015 at 2:05 PM, Xinliang David Li <davidxl at google.com>
> > wrote:
> >>
> >>
> >>
> >> On Thu, Nov 12, 2015 at 1:30 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>>
> >>>
> >>>
> >>> On Thu, Nov 12, 2015 at 1:25 PM, Xinliang David Li <davidxl 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.
> >>>>
> >>>>
> >>>> That is not the case -- there is no guarantee that the comdat defs are
> >>>> available externally.
> >>>
> >>>
> >>> What I'm wondering is if we can make that guarantee - since we can see
> >>> multiple instances of a comdat it seems like we might be able to
> benefit
> >>> from only producing the definition of them in one place by changing the
> >>> linkages.
> >>
> >>
> >> Or waste a little compile time by always emitting comdats. Picking one
> >> file to emit comdat is probably doable -- but may cost compile time in
> the
> >> serial step of thinLTO which should be avoided.
> >
> >
> > I'm not sure I follow - you'd just pick the first one/any one, no? So
> far as
> > I can see it's just "there are N instances of this comdat, pick one at
> > random and promote linkonce_odr -> weak_odr, convert the rest to
> > available_externally" (or don't convert the rest, but only make newly
> > imported versions available externally if that's a compile time cost to
> > revisit the existing instances of the comdat)
>
> ThinLTO doesn't by default need to read in all of the IR in the serial
> step (it just combines the per-module function summaries used for
> importing decisions into a single global summary). The backend jobs
> which do the importing are all independent and run in parallel (one
> per original module).
>

Ah, OK - thanks for the details.

> To make these linkage changes would require ThinLTO to do more in the
> serial step than just parsing out the function summaries and combining
> them.

Right - as it's parsing the summaries it could add a flag to the first
instance of each linkonce_odr function so that it could be changed to
weak_odr in the parallel importing phase.

> We might want to do more optimizations like this eventually in
> the ThinLTO plugin/serial step, but probably not by default in order
> to keep it as minimal as possible.

Given the extra work (code & computation/etc) of duplicating other comdat
members it doesn't seem so clear that the duplication option is the cheaper
(either coding or computation) of the two, though, no?

& it adds link time (to deduplicate the comdats) which is also serial-ish
(& larger object files to ship around to the link machine)

> I suppose we could do something
> like this just via summaries (record linkage types of functions in the
> summaries, do some analysis in the ThinLTO plugin step, write out the
> new linking types in the global summary, make the changes in the
> parallel backends. But that isn't something we are ready to do at this
> point, so it needs to work correctly without that support.
>

Anyway, leave it up to you folks to sort out. Just struck me as
strange/noteworthy.

Thanks for the explanations & such!
- Dave

>
> >
> > - Dave
> >
> >>
> >>
> >> David
> >>
> >>
> >>>
> >>>
> >>>
> >>> - David
> >>>
> >>>>
> >>>>
> >>>> David
> >>>>
> >>>>>
> >>>>>
> >>>>> I guess I'm missing something - is there a pointer to when/why this
> is
> >>>>> necessary?
> >>>>>
> >>>>> (& 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)
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> 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
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
> >
> > _______________________________________________
> > 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/76626539/attachment.html>