<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 25, 2017 at 9:03 PM Mehdi AMINI via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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><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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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><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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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><div><br>Looks stale, indeed - will update.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: test/ThinLTO/X86/alias_import.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-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><div><br>There was a conversation on another thread with Peter (& an in person conversation ) - see r274699 on the commits list.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: test/Transforms/FunctionImport/funcimport.ll:43<br>
<br>
 ; Aliases import the aliasee function<br>
 declare void @linkoncealias(...) #1<br>
----------------<br>
Comment seems out-of-date?<br></blockquote><div><br>Indeed. Will update - thanks!<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D35875" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35875</a><br>
<br>
<br>
<br>
</blockquote></div></div>