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

[Teresa Johnson via llvm-commits]

On Thu, Nov 12, 2015 at 2:29 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> 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.

Note that linkonce can't be discarded if they are referenced (and are
already discarded if unreferenced), so this would really only be a win
if we could convert one to weak and the rest to available_externally.

>
>>
>> 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)

Yes I think this is an interesting thing to explore. It will need more
work, and the current linkage changes and the comdat handling here are
still required based on what is coming in to the backend right now
(currently only via llvm-link based tests in fact as the importing
pass has not been upstreamed yet).

>
>>
>> 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!

No problem and thanks for the questions, this is another good thing we
can try once we have the whole framework in place!

Teresa

> - 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
>
>



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