<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2017-07-25 21:32 GMT-07:00 David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span class=""><div dir="ltr">On Tue, Jul 25, 2017 at 9:27 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br></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">2017-07-25 21:17 GMT-07:00 David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span class="m_1438855193119545197m_-1631295753644954835m_6195966376155617792gmail-"><div dir="ltr">On Tue, Jul 25, 2017 at 9:03 PM Mehdi AMINI via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">mehdi_amini added a comment.<br>
<br>
> they had to be emitted linkonce_odr in all the destination modules (even if they weren't used by an alias) rather than as available_externally - causing extra object size.<br>
<br>
Can you clarify this? It isn't clear to me.<br></blockquote></span><div><br>Functions imported as linkonce_odr, if they aren't optimized away, must be emitted into the resulting object file. (whereas if they're imported available_externally, if they aren't optimized away (inlined into all call sites, etc) they can be dropped and the external definition can be relied on)<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Right, but the part I don't connect to is "even if they weren't used by an alias"? We're talking about function that are imported to be referenced by an alias, if they are not "used by an alias" what's their use then? </div></div></div></div></blockquote></span><div><br>Ah, no - this logic seems to apply to all linkonce_odr imported functions. Yes, the impact could be improved/narrowed down to only apply when imported for an alias. (though it'd still have the problems for debug info, which is admittedly my original concern)<br><br>The reduced test case where I hit this looked something like this:<br><br>foo.cpp:<br>  inline __attribute__((optnone)) void f1() { }<br>  __attribute__((always_inline)) void f2() { f1(); }<br>bar.cpp:<br>  void f2();<br>  int main() { f2(); }<br><br>f2 got imported and optimized away, f1 was imported but couldn't be optimized away (due to optnone) & was emitted into bar.o as linkonce_odr (& created a second CU to put it in).<br></div></div></div></blockquote><div><br></div><div>That's not what I expect.</div><div><br></div><div>I would need to double check but we are supposed to import linkonce_odr as available_externally except when they are reference by an alias.</div><div>So in your case it shouldn't be emitted in bar.cpp since it should be imported as available_externally.</div><div><br></div><div>We "promote" linkonce_odr to weak_odr in their source module (foo.cpp in your case)  when their caller is imported somewhere else because of this: f2() inlined into main() will introduce a reference to f1() and it is no longer allowed to be discarded in foo.cpp.</div><div><br></div><div><br></div><div>-- </div><div>Medhi</div><div><br></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_quote"><div> </div><span class=""><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></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span class="m_1438855193119545197m_-1631295753644954835m_6195966376155617792gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
> There's no reason to believe this particularly narrow alias importing was especially/meaningfully important, only that it was /possible/ to implement in this way.<br>
<br>
Do you have benchmark results (SPEC and your internal ones)? Also statistics on the number of imports before/after?<br></blockquote></span><div><br>Nope - though I don't think there was any data to support the original feature except the observation that it was possible in this particular case, not that it was a common case, etc.<br><br>I could probably create some way to gather statistics, might take on the order of hours to build/gather/etc. My rough guess is that it's not worth it to do, but certainly open to other perspectives.<br><br>We'll certainly be running numbers on this internally (in the sense that we'd notice fi it breaks anything). I can frontload that (by asking Teresa & co what the best path is to do such an assessment internally) if you think the risk is high enough to be worth it.<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I don't know, I leave this assessment up to Teresa.  I was expecting that you guys have facilities to collect this information very easily :)</div></div></div></div></blockquote></span><div><br>Fair - I'll let Teresa speak to this (I haven't been dabbling much with the optimization side of ThinLTO until recently, so not sure what the state of the art/preferred level of validation is).<br> </div><span class=""><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></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span class="m_1438855193119545197m_-1631295753644954835m_6195966376155617792gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
<br>
<br>
================<br>
Comment at: test/Linker/funcimport.ll:63<br>
 ; An alias to an imported function is imported as alias if the function is not<br>
 ; available_externally.<br>
 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=linkoncefunc:%t.bc -S | FileCheck %s --check-prefix=IMPORTGLOB5<br>
----------------<br>
Is this comment still up to date?<br></blockquote></span><div><br>Looks stale, indeed - will update.<br> </div><span class="m_1438855193119545197m_-1631295753644954835m_6195966376155617792gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
<br>
================<br>
Comment at: test/ThinLTO/X86/alias_import.<wbr>ll:5<br>
 ; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t.index.bc %t2.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=PROMOTE<br>
-; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t.index.bc %t2.bc -o - | llvm-lto -thinlto-action=internalize -thinlto-index %t.index.bc -thinlto-module-id=%t2.bc - -o - | llvm-dis -o - | FileCheck %s --check-prefix=PROMOTE-<wbr>INTERNALIZE<br>
 ; RUN: llvm-lto -thinlto-action=import -thinlto-index %t.index.bc %t1.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT<br>
----------------<br>
It isn't clear to me that what is tested here is obsolete by your patch.<br></blockquote></span><div><br>There was a conversation on another thread with Peter (& an in person conversation ) - see r274699 on the commits list. </div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>OK! LGTM.</div></div></div></div></blockquote></span><div><br>Thanks for checking/looking/reviewing (:<br> </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></div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- </div><div>Mehdi</div><div><br></div></font></span></div></div></div></blockquote></div></div>
</blockquote></div><br></div></div>