<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 6, 2018 at 5:11 PM, Vlad Tsyrklevich via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</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">vlad.tsyrklevich added inline comments.<br>
<span><br>
<br>
================<br>
Comment at: lib/Transforms/IPO/FunctionImp<wbr>ort.cpp:620<br>
</span><span>+      for (auto &S : VI.getSummaryList())<br>
+        if (S->linkage() != GlobalValue::AvailableExternal<wbr>lyLinkage)<br>
+          return;<br>
</span>----------------<br>
<span>pcc wrote:<br>
> tejohnson wrote:<br>
> > Is it possible to have a symbol that is weak in one module and available_externally in another? If so, it would be more correct to return only if none of the copies is available_externally.<br>
> I suppose so. In the situation where we have at least one available-externally we could end up needing a symbol referenced by the available-externally as a result of a backend inlining it into a caller. So we would need to keep them live anyway.<br>
><br>
> Now that I think about it, this applies to linkonce-odr as well (i.e. we could convert them to available-externally as a result of non-prevailing, and then inline them in the backend and thus end up needing a symbol that was referenced by the available-externally but not necessarily by the prevailing copy of the global). But that's somewhat of an orthogonal problem to this.<br>
</span>As I mentioned to @pcc, his example would not be true for ThinLTO as of D42107 because it deletes dead symbols in lto::thinBackend. I'm not sure if it's possible to reach that case with regular LTO.<br>
<br>
@tejohnson I think that case is technically possible; however, it would re-introduce the same bug that D42107 originally tried to fix: <a href="https://bugs.llvm.org/show_bug.cgi?id=35938" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug<wbr>.cgi?id=35938</a></blockquote><div><br></div><div>Hmm, good point. The issue will happen if there is no prevailing copy in the IR (i.e. prevailing copy is in a native object), and we have in the IR both an available_externally and weak copy.<br></div><div><br></div><div>With your current patch we will see the weak copy, and thus not mark GUID as live, and you will end up with the same bug you are trying to solve here for the module containing the available_externally copy.</div><div><br></div><div>With my proposed change (return only if no available_externally copies), we will mark GUID live, and we will later come along in thinLTOInternalizeAndPromoteGU<wbr>ID and internalize it, which incorrectly allows it to be inlined (as in PR35938). And if we didn't do that, in thinLTOResolveWeakForLinkerGUI<wbr>D we would see that the weak symbol is not prevailing and mark it as available_externally, also allowing it to be inlined.</div><div><br></div><div>However, the bug PR35938 is only an issue when the non-prevailing symbol has interposable linkage (i.e. WeakAny or LinkonceAny), where the non-IR prevailing copy could be different. Is it even possible that we end up with an available_externally copy of the symbol as well? It would have to be the same as the prevailing copy for behavior to be sensible, and I'm not sure how you end up in that situation. I was thinking more about the weak/linkonce ODR case, where all copies are guaranteed to be the same. In that case, PR35938 doesn't come into play. In that case, then I think the correct behavior is to treat it as dead only if none of the copies is available_externally. If any is available_externally, then it would be ok to mark the GUID as live, since it would be legal to internalize and inline the weak/linkonce ODR copies.</div><div><br></div><div>Perhaps the right approach would be to change this to return only if none of the copies are available_externally, and additionally assert that we don't have a combination of available_externally and interposable copies.</div><div><br></div><div>Teresa</div><div><br></div><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"><br>
<br>
Give that, I'm not sure if there's a way to reconcile that change with this one. I'm not sure if marking individual symbols live/not-live is feasible?<br>
<div class="m_-6231166679787570472m_7122473869280626428gmail-m_-240283854776641944m_-80298804129990238HOEnZb"><div class="m_-6231166679787570472m_7122473869280626428gmail-m_-240283854776641944m_-80298804129990238h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D43690" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4369<wbr>0</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="m_-6231166679787570472m_7122473869280626428gmail-m_-240283854776641944m_-80298804129990238gmail_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>