[PATCH] D18986: [ThinLTO] Prevent importing of "llvm.used" values

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:219
@@ +218,3 @@
+    if (!V->hasLocalLinkage())
+      continue;
+    // We would have blocked importing from this module by suppressing index
----------------
tejohnson wrote:
> joker.eph wrote:
> > tejohnson wrote:
> > > joker.eph wrote:
> > > > I feel we should check `GlobalsToImport` here, the client has the ability to ask for promotion for specific symbols right? 
> > > But as I mentioned earlier, the problem comes into play with local symbols that are not in the GlobalsToImport set (but are rather referenced by a function in the GlobalsToImport set, requiring the promotion).
> > `GlobalsToImport` should be renamed into `GlobalsToPromote` indeed, this is how it is intended to be used right? You already gets wrong result if you don't fill it with referenced symbols or is there some magic I missed? (I fill the "export lists" with the referenced symbols for this purpose).
> No, the supplied GlobalsToImport is only used to distinguish what is being imported as a definition vs only a declaration - it is used to select the proper linkage type (e.g. available_externally vs external). It isn't used to drive promotion. And in fact FunctionImporter::importFunctions only places the imported GVs in this set. And the set isn't used when running this on the exporting module (see paragraph after next).
> 
> When importing, we promote any referenced local value, which makes sense since those are exactly the ones we need to promote since they are now being referenced from another module (at least until we implement an optimization that decides when it is better to import a local as a definition and leave it local, but that is orthogonal to this issue).
> 
> On the exporting side, which is where the code added here by the patch is handling, we don't pass in a GlobalsToImport set, since it isn't relevant. In order to minimize promotions on the exporting side, we need to know what references are being exported, as you mention here. That should be passed in as a separate argument. At that point, we could intersect the values on the llvm.used set with those in the supplied exported references set as a sanity check that none of the llvm.used values ended up exported. 
> 
> Note that if we want to do a finer grained approach to handling inline asm, preventing importing of just those functions containing the inline assembly, we will either have to implement the support to pass in the exported symbols here, or add an alias to the original name for anything in llvm.used. In the former case the reference graph would have to be augmented to add references from all functions with inline assembly to all the llvm.used values (see my other note here on why CollectAsmUndefinedRefs can't be used to get a more accurate indication on which inline assembly uses which values).
Reminds me that I started splitting `FunctionImportGlobalProcessing` into two part: one to perform the linkage changes required before importing, and the other doing the promotion.


http://reviews.llvm.org/D18986