[PATCH] D26076: [ThinLTO] Correctly resolve linkonce when importing aliasee

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D26076#582320, @mehdi_amini wrote:

> CC Rafael to hint here.
>
> > When we have an aliasee that is linkonce, while we can't convert the non-prevailing copies to available_externally
>
> It is not clear to me why? I understand that the verifier won't like now, but why isn't possible in theory?
>
> Also even if we wouldn't want to relax this in general, for ThinLTO we can always clone the function into an available_externally that would replace the alias.


Yes we could do this, and we've talked about doing this before (see the comment where we compute the GlobalInvolvedWithAlias set just below in llvm::thinLTOResolveWeakForLinkerInIndex). That is a longer term enhancement to handle this limitation of not being able to convert non-prevailing aliasees to available_externally. But note this patch doesn't affect that case, it is rather dealing with a missing case for the prevailing copy.



================
Comment at: lib/LTO/LTO.cpp:123
 
 static void thinLTOResolveWeakForLinkerGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
----------------
mehdi_amini wrote:
> It is not clear to me right now why is it part of this stage. Conceptually it seems that you are solving a "promotion" issue, and not a "weak resolution" thing.
The code below here converts the prevailing linkonce to weak to ensure one copy is kept - even if there are no other copies that will be converted to available_externally. This is not new and ensures if we export a reference to that linkonce that it will be satisfied at link time (so that particular part of this routine is in fact like a promotion).

The change in this patch is to allow that part to occur (the correctness part) for an aliasee. It was a missing case, and was resulting in some undefs at link time in certain cases (e.g. the unsats davidxl got when trying to link Clang with ThinLTO + PGO last week).

We could either just better document this part of the handling in this routine, or split it out - e.g a separate pass of promoting linkonce->weak when the linkonce is exported, but that seems orthogonal to fixing the existing handling for this specific case (and somewhat redundant with the linkonce->weak that is done here already).


https://reviews.llvm.org/D26076