<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 26, 2017 at 6:47 AM Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.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">On Tue, Jul 25, 2017 at 10:43 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span class="m_-8025413160141518576gmail-"><div dir="ltr">On Tue, Jul 25, 2017 at 10:02 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span class="m_-8025413160141518576gmail-m_8400445478429505901m_-5205179252906575522m_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:1px solid 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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div></blockquote></span></div></div></blockquote><div> </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>That's not what was actually happening though. We still had some code that was left over from when we forcibly imported linkonce_odr on reference  (before we did the weak resolution). That code is removed in David's patch.  See the change in FunctionImportGlobalProcessing::getLinkage which removed the following case:</div><div><br></div><div><div>  case GlobalValue::LinkOnceAnyLinkage:</div><div>  case GlobalValue::LinkOnceODRLinkage:</div><div>    // These both stay the same when importing the definition.</div><div>    // The ThinLTO pass will eventually force-import their definitions.</div><div>    return SGV->getLinkage();</div></div><div><br></div><div>The second part of the comment there is stale and wrong now, but never got cleaned up. This code is what sets the linkage type on the values being imported. Conversely, the weak resolution adjusts the linkage type on the values already in the module (and runs on the module in the backend *before* we do any importing).</div><div><br></div><div>The fact that David is removing the above handling is what is requiring the other part of this patch - removing the special casing of handling for linkonce_odr aliases. </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span class="m_-8025413160141518576gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><br><br><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></div></div></blockquote></span><div><br>Yeah, I'm certainly confused as well - and internally (where I was testing most of this, though I was debugging the final/specific backend actions with an open source debug built clang) I'm not seeing that promotion to available_externally even in simpler cases (one header with an optnone inline function, two callers in separate .cc files, all ThinLTO'd together). I do see an internal definition on one side and a weak (which could be linkonce_odr or weak_odr at the LLVM level, they both get lowered to ELF weak) definition on the other side, according to nm.<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Use -Wl,-plugin-opt,save-temps (for gold at least) to get the bitcode out after each thinlto optimization to see the linkage types we change them to. The ones named .*.promote.bc include both promotion to global and the weak resolution changes, then .*.internalize.bc contain the subsequent internalization changes, etc.</div><div> </div><div>I wonder if the thinlto internalization is coming along and deciding to change the non-prevailing available_externally copy to internal linkage since it isn't exported. That would be a bug. Hmm, looking at thinLTOInternalizeAndPromoteGUID() it certainly seems like we could be doing that, I don't see a guard against converting available_externally to internal...</div></div></div></div></blockquote><div><br>Thanks for the in person help - for posterity: We figured it out: My example was degenerate. The second use of the inline function was in a dead object file, which in the distributed ThinLTO case causes that object file to be produced as normal, without any use of a summary. So it got a normal linkonce_odr definition of the inline function and strong definition of the caller to it (& since this object wasn't considered at all, the linkonce_odr function was correctly internalized in the other object since that was the only non-dead use of it). Once we fixed that we saw the expected behavior: weak in one object, undefined (from available_externally) in the other object.<br><br>Carry on... <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><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>Just more mysteries :/<br><br>- Dave<br> </div><div><div class="m_-8025413160141518576gmail-h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div><br></div><div>-- </div><div>Medhi</div></div></div></div><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:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span class="m_-8025413160141518576gmail-m_8400445478429505901m_-5205179252906575522m_1438855193119545197m_-1631295753644954835m_6195966376155617792gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid 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><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span class="m_-8025413160141518576gmail-m_8400445478429505901m_-5205179252906575522m_1438855193119545197m_-1631295753644954835m_6195966376155617792gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid 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_-8025413160141518576gmail-m_8400445478429505901m_-5205179252906575522m_1438855193119545197m_-1631295753644954835m_6195966376155617792gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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></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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><span class="m_-8025413160141518576gmail-m_8400445478429505901m_-5205179252906575522HOEnZb"><font color="#888888"><div><br></div><div>-- </div><div>Mehdi</div><div><br></div></font></span></div></div></div></blockquote></div></div>
</blockquote></div></div></div></blockquote></div></div></div></div>
</blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><br><br clear="all"><div><br></div>-- <br><div class="m_-8025413160141518576gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"> <a href="tel:(408)%20460-2413" value="+14084602413" target="_blank">408-460-2413</a></td></tr></tbody></table></span></div>
</div></div></blockquote></div></div>