<div dir="ltr">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.<div><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 12, 2015 at 2:06 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Nov 12, 2015 at 2:05 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@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 dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 12, 2015 at 1:30 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 12, 2015 at 1:25 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@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 dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 12, 2015 at 12:56 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br></span><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 12, 2015 at 11:41 AM, Teresa Johnson via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 the<br>
comdat selection type correct it is significantly simpler to import the<br>
full comdat group in a single pass.<br></blockquote><div><br></div></span><div>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.<br></div></div></div></div></blockquote><div><br></div></span><div>That is not the case -- there is no guarantee that the comdat defs are available externally.</div></div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote><div><br></div></span><div>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)<br><br>- Dave</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span><font color="#888888"><br><br>- David</font></span></div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>I guess I'm missing something - is there a pointer to when/why this is necessary?<br><br>(& 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)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
<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 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 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></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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></blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>