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

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 28 12:46:09 PDT 2016


mehdi_amini added inline comments.


================
Comment at: lib/LTO/LTO.cpp:123
 
 static void thinLTOResolveWeakForLinkerGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
----------------
tejohnson wrote:
> tejohnson wrote:
> > 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).
> Note also this routine used to explicitly check for and handle the exported case, but it was removed with the idea that we would always do the linkonce->weak promotion for the prevailing copy (see r274722 and r274784).
> 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,

Right, splitting it seems better to me.

> 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).

Agree.


https://reviews.llvm.org/D26076





More information about the llvm-commits mailing list