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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D18986#404837, @tejohnson wrote:

> In http://reviews.llvm.org/D18986#404818, @joker.eph wrote:
>
> > In http://reviews.llvm.org/D18986#404816, @tejohnson wrote:
> >
> > > In http://reviews.llvm.org/D18986#404640, @joker.eph wrote:
> > >
> > > >
> > >
> >
>


[...]

> > > The inline asm string "movzbl myvar(..." is an opaque blob that the compiler doesn't AFAIK do any analysis on to decide what is a global value - the reason why the myvar ends up in llvm.used is only because of the __attribute__((used)) on the myvar def, not because the compiler detects the reference within an inline assembly string.

> 

> > 

> 

> > 

> 

> > See `CollectAsmUndefinedRefs` in http://reviews.llvm.org/D19103 for the analysis performed to parse the assembly and update llvm.used.

> 

> >  But that's not relevant to the point I was making (see below).

> 

> 

> Interesting, I didn't realize there was some parsing/introspection of the inline assembly beyond the register assignment.


I looked at this functionality and it doesn't handle this type of inline assembly. Confirmed by walking through the similar handling in IRObjectFile on which CollectAsmUndefinedRefs was modeled. It only handles "Module-level assembly" as described here: http://llvm.org/docs/LangRef.html#module-level-inline-assembly.

So if we instead want to use a finer grained approach here, I think we need to conservatively reject importing for any function containing inline assembly if there is any llvm.used.

[...]

> > The big hammer seems reasonable to me on the very short term.

> 


This is my preference given what I think needs to be done to get this to work on a finer grained level vs how uncommon I think the inline assembly is in practice. It can be refined if we find important cases from modules with inline assembly.

> > Preventing the import of these function can be done using a "flag" in the summary (part of the flags we already talked about, i.e. optnone, optsize, etc.).

> 


To get the finer-grained solution to work, we need to add this indication to the summary, and prevent importing of those functions. Then on the exporting side we need to add an alias to the old name for anything in the llvm.used set (even if we don't export any functions with inline assembly, a reference in another function could be exported, requiring the promotion).


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


http://reviews.llvm.org/D18986