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

Thu Nov 12 14:17:23 PST 2015

It is not that simple. With PGO things can be complicated. Currently we
also comdat profile counters of comdat functions which is not the best
thing to do (because post inline update of the counters get discarded). We
need to revisit that at some point. The bottom line is that it will also
affects how comdat copies are optimized and which out of line versions to
be picked.  Once that design is ready, it will be discussed in a different
thread.

David

On Thu, Nov 12, 2015 at 2:06 PM, David Blaikie <dblaikie at gmail.com> 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)
>
> - 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151112/90f92978/attachment-0001.html>