<div dir="ltr"><br><div class="gmail_extra"><br><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_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>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><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_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>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><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_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_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-INTERNA<wbr>LIZE<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>OK! LGTM.</div><div><br></div><div><br></div><div>-- </div><div>Mehdi</div><div><br></div></div></div></div>