[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#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?



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:426
+                                            bool PerModuleIndex = true,
+                                            bool AssertExists = true) const {
     assert(GV.hasName() && "Can't get GlobalValueSummary for GV with no name");
----------------
mehdi_amini wrote:
> I'm not convinced by this API: the extra parameter corresponds exactly to what is usually exposed as a separate API (like `std::vector::get` vs `std::vector::operator[]` for example)
> 
I removed this change and modified the use sites to instead use the existing findGlobalValueSummaryList.


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


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:243
 
+  // Collect the set of llvm.compiler.used locak variables, which along with the
+  // llvm.used variables already in the LocalUsed set, cannot be renamed as
----------------
mehdi_amini wrote:
> s/locak/local/
fixed


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:276
+          if (!(Flags & (object::BasicSymbolRef::SF_Weak ||
+                         object::BasicSymbolRef::SF_Global))) {
+            GlobalValue *GV = M.getNamedValue(Name);
----------------
mehdi_amini wrote:
> Early exit
fixed


https://reviews.llvm.org/D26146