<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 12, 2015 at 2:21 PM, Teresa Johnson <span dir="ltr"><<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Nov 12, 2015 at 2:06 PM, David Blaikie via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
><br>
> On Thu, Nov 12, 2015 at 2:05 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>><br>
>><br>
>> On Thu, Nov 12, 2015 at 1:30 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>><br>
>>><br>
>>> On Thu, Nov 12, 2015 at 1:25 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>><br>
>>>><br>
>>>> On Thu, Nov 12, 2015 at 12:56 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>> On Thu, Nov 12, 2015 at 11:41 AM, Teresa Johnson via llvm-commits<br>
>>>>> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>>>>><br>
>>>>>> tejohnson created this revision.<br>
>>>>>> tejohnson added reviewers: rafael, dexonsmith.<br>
>>>>>> tejohnson added subscribers: llvm-commits, davidxl, joker.eph.<br>
>>>>>><br>
>>>>>> A number of comdat related fixes, cleanup and test cases.<br>
>>>>>><br>
>>>>>> I made a number of changes to get importing working when there are<br>
>>>>>> comdats, with lots of new test cases involving comdats of various<br>
>>>>>> selection types, linkage types, with and without aliases.<br>
>>>>>><br>
>>>>>> The changes ensure we link in the full comdat group containing the<br>
>>>>>> function indicated for importing. Previously I intended to require<br>
>>>>>> that the importer specify the additional comdat group members for<br>
>>>>>> importing on successive importing requests. However, in order to get<br>
>>>>>> the<br>
>>>>>> comdat selection type correct it is significantly simpler to import<br>
>>>>>> the<br>
>>>>>> full comdat group in a single pass.<br>
>>>>><br>
>>>>><br>
>>>>> I thought all imported definitions were marked available_externally -<br>
>>>>> in which case I'm not sure why it would be necessary to bring in the whole<br>
>>>>> comdat group, since the comdat would never be emitted here.<br>
>>>><br>
>>>><br>
>>>> That is not the case -- there is no guarantee that the comdat defs are<br>
>>>> available externally.<br>
>>><br>
>>><br>
>>> What I'm wondering is if we can make that guarantee - since we can see<br>
>>> multiple instances of a comdat it seems like we might be able to benefit<br>
>>> from only producing the definition of them in one place by changing the<br>
>>> linkages.<br>
>><br>
>><br>
>> Or waste a little compile time by always emitting comdats. Picking one<br>
>> file to emit comdat is probably doable -- but may cost compile time in the<br>
>> serial step of thinLTO which should be avoided.<br>
><br>
><br>
> I'm not sure I follow - you'd just pick the first one/any one, no? So far as<br>
> I can see it's just "there are N instances of this comdat, pick one at<br>
> random and promote linkonce_odr -> weak_odr, convert the rest to<br>
> available_externally" (or don't convert the rest, but only make newly<br>
> imported versions available externally if that's a compile time cost to<br>
> revisit the existing instances of the comdat)<br>
<br>
</div></div>ThinLTO doesn't by default need to read in all of the IR in the serial<br>
step (it just combines the per-module function summaries used for<br>
importing decisions into a single global summary). The backend jobs<br>
which do the importing are all independent and run in parallel (one<br>
per original module).<br></blockquote><div><br></div><div>Ah, OK - thanks for the details.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">To make these linkage changes would require ThinLTO to do more in the<br>
serial step than just parsing out the function summaries and combining<br>
them.</blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> We might want to do more optimizations like this eventually in<br>
the ThinLTO plugin/serial step, but probably not by default in order<br>
to keep it as minimal as possible.</blockquote><div><br></div><div>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?<br><br>& it adds link time (to deduplicate the comdats) which is also serial-ish (& larger object files to ship around to the link machine)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I suppose we could do something<br>
like this just via summaries (record linkage types of functions in the<br>
summaries, do some analysis in the ThinLTO plugin step, write out the<br>
new linking types in the global summary, make the changes in the<br>
parallel backends. But that isn't something we are ready to do at this<br>
point, so it needs to work correctly without that support.<br></blockquote><div><br></div><div>Anyway, leave it up to you folks to sort out. Just struck me as strange/noteworthy.<br><br>Thanks for the explanations & such!<br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> - Dave<br>
><br>
>><br>
>><br>
>> David<br>
>><br>
>><br>
>>><br>
>>><br>
>>><br>
>>> - David<br>
>>><br>
>>>><br>
>>>><br>
>>>> David<br>
>>>><br>
>>>>><br>
>>>>><br>
>>>>> I guess I'm missing something - is there a pointer to when/why this is<br>
>>>>> necessary?<br>
>>>>><br>
>>>>> (& in fact, it might be useful to have this pass make one instance of a<br>
>>>>> comdat group into an external definition (I guess that's probably not<br>
>>>>> possible unless you have true whole program knowledge - maybe there's<br>
>>>>> another linkage type which would produce the comdat group even if all the<br>
>>>>> calls are optimized away) - and mark all the other instances<br>
>>>>> available_externally? even potentially stripping the comdat groups from<br>
>>>>> modules where the call sites are really cold)<br>
>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> Additionally, if the module linker selects the source version of a<br>
>>>>>> comdat not containing the imported function, if it is referenced<br>
>>>>>> by an imported function we also import all its comdat group member<br>
>>>>>> definitions (via the lazy linker) in the same importing pass.<br>
>>>>>> In order to support this, we are now much more aggressive about lazy<br>
>>>>>> linking global values during importing, which only need to be<br>
>>>>>> linked in (as either declarations or definitions) if referenced by an<br>
>>>>>> imported function.<br>
>>>>>><br>
>>>>>> Also, because we may now import additional function bodies lazily, it<br>
>>>>>> is<br>
>>>>>> no longer possible to determine up front whether an alias's aliasee is<br>
>>>>>> going to be imported as a definition or not when we copy its<br>
>>>>>> prototype.<br>
>>>>>> Therefore, we now convert aliases into declarations after all lazy<br>
>>>>>> linking is complete, if the aliasee definition was not imported.<br>
>>>>>><br>
>>>>>> Finally, since comdat members may not be declarations (including<br>
>>>>>> available_externally), after linking is complete those that were<br>
>>>>>> imported as linker declarations will be removed from comdats.<br>
>>>>>><br>
>>>>>> <a href="http://reviews.llvm.org/D14623" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14623</a><br>
>>>>>><br>
>>>>>> Files:<br>
>>>>>>   lib/Linker/LinkModules.cpp<br>
>>>>>>   test/Linker/Inputs/funcimport_comdat.ll<br>
>>>>>>   test/Linker/Inputs/funcimport_comdat2.ll<br>
>>>>>>   test/Linker/Inputs/funcimport_comdat3.ll<br>
>>>>>>   test/Linker/funcimport.ll<br>
>>>>>>   test/Linker/funcimport_comdat.ll<br>
>>>>>>   test/Linker/funcimport_comdat2.ll<br>
>>>>>>   test/Linker/funcimport_comdat3.ll<br>
>>>>>><br>
>>>>>><br>
>>>>>> _______________________________________________<br>
>>>>>> llvm-commits mailing list<br>
>>>>>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>>>>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>>>>>><br>
>>>>><br>
>>>><br>
>>><br>
>><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
<br>
<br>
</div></div><div class="HOEnZb"><div class="h5">--<br>
Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com">tejohnson@google.com</a> | <a href="tel:408-460-2413" value="+14084602413">408-460-2413</a><br>
</div></div></blockquote></div><br></div></div>