[PATCH] D25854: [lto] Kill undefined extern_weak declarations before opt

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: lib/LTO/LTO.cpp:346
       continue;
     if (Res.Prevailing && GV) {
       Keep.push_back(GV);
----------------
mehdi_amini wrote:
> pcc wrote:
> > mehdi_amini wrote:
> > > pcc wrote:
> > > > I would have thought that you could keep track of the undefined symbols in a local variable here and RAUW them in a loop below. That way, you can avoid the global tracking. 
> > > How do you address ThinLTO?
> > > 
> > > Which needs to be added to the patch BTW.
> > > How do you address ThinLTO?
> > 
> > The way I see this feature eventually being added to ThinLTO is:
> > - decouple LLVM linkages from ThinLTO summary linkages
> > - (orthogonally: represent "drop" as another type of linkage -- this would allow us to drop unchosen linkonce or weak (non-odr) definitions)
> > - add a new "linkage" (at this point we can probably call it a resolution) which represents an undefined extern_weak.
> > - copy all resolution information into the summary during addThinLTO while handling each input file (during mergeFrom or thereabouts)
> > 
> > > Which needs to be added to the patch BTW.
> > 
> > It doesn't seem necessary to do so in the first patch. This is just an optimisation, and the CFI feature that relies on this does not (currently) work with ThinLTO. But @eugenis please do add a FIXME.
> > The way I see this feature eventually being added to ThinLTO is [...]
> 
> Please send an RFC for such kind of changes.
> 
> > It doesn't seem necessary to do so in the first patch. 
> 
> I don't like this. One of the main reason I wanted a unified API is also exactly to avoid these kinds of discrepancies and designed these from the ground-up for LTO *and* ThinLTO.
> 
To clarify my position, I'm fine with an "incremental" patch, as long as we first validate a design that handle both, i.e. not having ThinLTO folks (including me) arriving after the fact and having to basically redo everything from scratch because the way it is done cannot integrate with ThinLTO.


Repository:
  rL LLVM

https://reviews.llvm.org/D25854