[PATCH] D43690: [ThinLTO] Keep available_externally symbols live

[Teresa Johnson via llvm-commits]

On Tue, Mar 6, 2018 at 5:11 PM, Vlad Tsyrklevich via Phabricator <
reviews at reviews.llvm.org> wrote:

> vlad.tsyrklevich added inline comments.
>
>
> ================
> Comment at: lib/Transforms/IPO/FunctionImport.cpp:620
> +      for (auto &S : VI.getSummaryList())
> +        if (S->linkage() != GlobalValue::AvailableExternallyLinkage)
> +          return;
> ----------------
> pcc wrote:
> > tejohnson wrote:
> > > 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.
> > 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.
> >
> > 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.
> 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.
>
> @tejohnson I think that case is technically possible; however, it would
> re-introduce the same bug that D42107 originally tried to fix:
> https://bugs.llvm.org/show_bug.cgi?id=35938


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.

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.

With my proposed change (return only if no available_externally copies), we
will mark GUID live, and we will later come along in
thinLTOInternalizeAndPromoteGUID and internalize it, which incorrectly
allows it to be inlined (as in PR35938). And if we didn't do that, in
thinLTOResolveWeakForLinkerGUID we would see that the weak symbol is not
prevailing and mark it as available_externally, also allowing it to be
inlined.

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.

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.

Teresa



>
> 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?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D43690
>
>
>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
<(408)%20460-2413>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180307/3bb543d2/attachment.html>