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

Thu Nov 12 14:21:55 PST 2015

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

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

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