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

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

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?




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"). 
(for clang, this is done by the user who usually in C++ uses `__attribute__((used))`, which translates into llvm.used)



================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:208
       LocalsUsed.insert(V);
   }
 
----------------
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". 


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:256
+  }
+
   for (auto *V : LocalsUsed) {
----------------
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.


https://reviews.llvm.org/D26146