[PATCH] D26146: [ThinLTO] Prevent exporting of locals used/defined in module level asm

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D26146#589364, @mehdi_amini wrote:

> In https://reviews.llvm.org/D26146#589341, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D26146#585633, @mehdi_amini wrote:
> >
> > > > Then the assumption works for this fix since it is only looking for static globals (i.e. local linkage), right? I can just update the description and possibly the code comment if that is the case
> > >
> > > The `attribute((used))` will makes use of the `@llvm.used`, which you already handled before this patch right?
> >
> >
> > Right for llvm.used, but I was referring to static (local linkage) values in the llvm.compiler.used which is added in this patch, since you mentioned: "It does not apply to non-static I believe, and this is why are currently parsing the module inline ASM at LTO time and updating the llvm.compiler_used, because this is not something the FE is required to do."
> >
> > I read from that that any statics would have been placed by the FE in the llvm.compiler.used set, or is that not a correct interpretation? Going back to the email discussion I included in my earlier response, it seems that it must be true that "The frontend is responsible to add [local ref] @b to llvm.compiler_used", in which case the handling in this current patch that looks for locals in the llvm.compiler.used is correct, right?
>
>
> To be more precise: the FE can put uses from inline ASM to static symbol in either llvm.used or llvm.compiler_used (i.e. the latter should be "enough").


Only module-level asm, not "inline asm calls", which are opaque to the compiler and require a user attribute like:

> (for clang, this is done by the user who usually in C++ uses `__attribute__((used))`, which translates into llvm.used)

Let me know if I have addressed your concerns with my other responses below.



================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:208
       LocalsUsed.insert(V);
   }
 
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > Can you clarify what is specific here that wouldn't make us add here the "compiler_used" variables?
> > I updated the comment above this block to address this hopefully.
> My question was the opposite: why *only* llvm.used here, and not *also* llvm.compiler_used.
> 
> You wrote here "We collect them here because ...." and the same reasons should apply to "compiler_used". 
I thought I did answer that: "Uses in inline asm must be marked in the source with “attribute((used))”, which causes them to be included in the llvm.used set."  As you noted also, this causes those to go into the llvm.used (not llvm.compiler.used). I will make an explicit note here about these not being in the llvm.compiler.used set, and therefore not needing to affect functions that make inline asm calls.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:256
+  }
+
   for (auto *V : LocalsUsed) {
----------------
mehdi_amini wrote:
> To be more clear with the above, this is the snippet I asked about why is it done here and not above around line 212.
Because as I noted in the comment added around the earlier handling, only the llvm.used set locals may be referenced by functions that make inline asm calls, and must be marked explicitly by the user via attribute((used)), which end up in llvm.used only. We only need to collect the llvm.compiler.used here to mark those as NoRename.


================
Comment at: test/ThinLTO/X86/module_asm2.ll:73
+  ret i32 %1
+}
----------------
mehdi_amini wrote:
> Can you add a comment to this file explaining what you're testing, maybe with each of the CHECK groups.
Will do.


https://reviews.llvm.org/D26146