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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

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

> In http://reviews.llvm.org/D18986#404640, @joker.eph wrote:
>
> > Yes this was my tentative distinction 1) and 2) above, i.e. promoting a function present in llvm.used vs importing a function that contains inline ASM.
>
>
> They are related: 1) (local value present on the llvm.used) means that 2) (importing a function with inline asm) has to be conservative as the inline asm may contain a reference to that local value.


I think functions in 1) can be totally decoupled, because as shown in my example previously you can always promote/import these regardless of functions in 2), because you can use a private alias.

> 

> 

> > We should be able to always promote AFAICT, but not import the one that contains Inline ASM if it has a reference to a local function.

> 

> 

> Except that you don't know which inline asm references which local function or variable from the llvm.used.


How does it matter? I thought we cleared it with my example earlier: you can still always promote with a private alias, as long as you never import a function with inline ASM it should be fine.

> For instance, the source code used to generate the example in this patch is:

> 

> static const unsigned char __attribute__((used)) __attribute__ ((aligned (1))) myvar = 1;

>  void foo(unsigned long int *v) {

> 

>   __asm__ volatile("movzbl     myvar(%%rip), %%eax\n\t"

>                    "movq %%rax, %0\n\t"

>                        : "=*m" (*v)

>     :

>     : "%eax"

>     );

> 

> }

> 

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

> But what I could do, which is a bit smaller of a hammer, is to prevent importing of any function that has any inline asm in it, if the module also contains an llvm.used with local values.


That was my point from the beginning: there is no issues with importing the function in llvm.used, it only an issue for the functions that defines the inline ASM.

> However, I thought that inline assembly would be rare enough to make this simpler workaround reasonable, especially since a longer term solution will be needed for the general issue on the LTO side (where this workaround won't help). I also have some concerns about getting it right if the decisions are more fine grained: e.g. if we decide to prevent importing for just those functions with inline assembly, do we suppress the importing by not emitting their summaries (might cause some complications if their linkage needs to be adjusted e.g. if they themselves are local and referenced by a different function that is imported thus requiring their promotion), or should they have summaries but mark them as no import somehow (in that case do their summaries need to conservatively reference all values in the llvm.used sets?), etc...haven't entirely thought through all the issues with implementing that approach.


The big hammer seems reasonable to me on the very short term.
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.).

> I am not entirely sure what other cases will use llvm.used besides inline assembly, are there other cases where we need to be conservative in the presence of one, because we won't have visibility into where the uses actually are or be able to rename them?


I don't know other cases for llvm.user. We also have llvm.compiler_used that needs to be preserved and can't be renamed, but they shouldn't be private either (?). I haven't thought much about these.


http://reviews.llvm.org/D18986