[PATCH] D28143: [ThinLTO] Thin link efficiency: Pre-compute set of non-exportable GVs

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 29 10:32:14 PST 2016


tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:144
   }
   return true;
 }
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > What about replacing all of this logic by a bit in the summary that would be set during the compile phase?
> > I like that idea. I started adding a flag this morning that was specific to this case (HasNotExportableReference). But it occurred to me - should we just subsume all checks done by eligibleToImport into a single flag like NotEligibleToImport? That would subsume the new flag as well as the NoRename, IsNotViableToInline and HasInlineAsmMaybeReferencingInternal flags. I.e. anything that today has any of the NoRename, IsNotViableToInline or HasInlineAsmMaybeReferencingInternal set would be marked NotEligibleToImport. Additionally, anything that references something that would today be marked NoRename would also be marked NotEligibleToImport (the case I want to add here).
> > 
> > With any of these new flags we'd need to bump the INDEX_VERSION. However, with the approach of subsuming all of these flags into one we'd need to conservatively always set NotEligibleToImport for old versions, whereas with a new flag we could reproduce it on the fly for older version bitcode out of the other flags (essentially do what the current eligibleToImport() routine does). Not sure how important it is to optimize for this case (handling the older bitcode) though.
> > 
> > WDYT?
> > But it occurred to me - should we just subsume all checks done by eligibleToImport into a single flag like NotEligibleToImport? 
> 
> That's what I had in mind :)
> 
> > hat would subsume the new flag as well as the NoRename, IsNotViableToInline and HasInlineAsmMaybeReferencingInternal flags. I.e. anything that today has any of the NoRename, IsNotViableToInline or HasInlineAsmMaybeReferencingInternal set would be marked NotEligibleToImport. 
> 
> I haven't thought about the exact list of flags that this would subsume, but I trust you to eliminate what's not needed!
> 
> > Not sure how important it is to optimize for this case (handling the older bitcode) though.
> 
> Not important IMO. We don't have a widespread adoption of ThinLTO.
> 
> That's what I had in mind :)

Ok, cool. Wanted to confirm before I started ripping things out too much. =) Will proceed.


https://reviews.llvm.org/D28143





More information about the llvm-commits mailing list