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



Repository:
  rL LLVM

https://reviews.llvm.org/D25854