[ThinLTO] Refine fix to avoid renaming of uses in inline assembly.

[tejohnson]

Summary

Refine the workaround from r266877 that attempts to prevent
renaming of locals in inline assembly, so that in addition to looking
for a llvm.used local value, that there is at least one inline assembly
call in the module. Otherwise, debug functions added to the llvm.used
can block importing/exporting unnecessarily.

[mehdi_amini]

LGTM.
This is sketchy though, I'm fine with it on the short term, but anything "clever" using the summary will assume that if you have a summary you have all the informations in the summary for the module somehow, which this is breaking AFAICT.

lib/Analysis/ModuleSummaryAnalysis.cpp

150 ↗

(On Diff #55143)

I think there is a shortcut for (auto &I : F.instructions())

[tejohnson]

In D19573#413169, @joker.eph wrote:

LGTM.
This is sketchy though, I'm fine with it on the short term, but anything "clever" using the summary will assume that if you have a summary you have all the informations in the summary for the module somehow, which this is breaking AFAICT.

I don't think it is quite that bad, since in this case we completely prevent building a summary for the module in question, so it should not have a different effect than linking it in as a native object file (where the rest of the bitcode files would have no ability to import from it etc either).

In any case, I do plan to move this to the more refined solution soon, but it isn't at the top of my heap just yet.

lib/Analysis/ModuleSummaryAnalysis.cpp

150 ↗

(On Diff #55143)

Thanks for the pointer (actually instructions(F), from InstIterator.h)

[tejohnson]

• Implement review suggestion (and rebase).

llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp

 
 #include "llvm/Analysis/ModuleSummaryAnalysis.h"
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/BlockFrequencyInfoImpl.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/InstIterator.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/ValueSymbolTable.h"
 #include "llvm/Pass.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "module-summary-analysis"
 
 // Walk through the operands of a given User via worklist iteration and populate
void ModuleSummaryIndexBuilder::computeVariableSummary(
   GVarSummary->addRefEdges(RefEdges);
   Index->addGlobalValueSummary(V.getName(), std::move(GVarSummary));
 }
 
 ModuleSummaryIndexBuilder::ModuleSummaryIndexBuilder(
     const Module *M,
     std::function<BlockFrequencyInfo *(const Function &F)> Ftor)
     : Index(llvm::make_unique<ModuleSummaryIndex>()), M(M) {
-  // We cannot currently promote or rename anything that is in llvm.used,
-  // since any such value may have a use that won't see the new name.
-  // Specifically, any uses within inline assembly are not visible to the
-  // compiler. Prevent importing of any modules containing these uses by
+  // We cannot currently promote or rename anything used in inline assembly,
+  // which are not visible to the compiler. Detect a possible case by looking
+  // for a llvm.used local value, in conjunction with an inline assembly call
+  // in the module. Prevent importing of any modules containing these uses by
   // suppressing generation of the index. This also prevents importing
   // into this module, which is also necessary to avoid needing to rename
   // in case of a name clash between a local in this module and an imported
   // global.
   // FIXME: If we find we need a finer-grained approach of preventing promotion
   // and renaming of just the functions using inline assembly we will need to:
   // - Add flag in the function summaries to identify those with inline asm.
   // - Prevent importing of any functions with flag set.
   // - Prevent importing of any global function with the same name as a
   //   function in current module that has the flag set.
   // - For any llvm.used value that is exported and promoted, add a private
   //   alias to the original name in the current module (even if we don't
   //   export the function using those values in inline asm, another function
   //   with a reference could be exported).
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(*M, Used, /*CompilerUsed*/ false);
+  bool LocalIsUsed = false;
   for (GlobalValue *V : Used) {
-    if (V->hasLocalLinkage())
-      return;
+    if ((LocalIsUsed |= V->hasLocalLinkage()))
+      break;
   }
+  if (LocalIsUsed)
+    for (auto &F : *M)
+      for (auto &I : instructions(F))
+        if (const CallInst *CallI = dyn_cast<CallInst>(&I))
+          if (CallI->isInlineAsm())
+            return;
 
   // Compute summaries for all functions defined in module, and save in the
   // index.
   for (auto &F : *M) {
     if (F.isDeclaration())
       continue;
 
     BlockFrequencyInfo *BFI = nullptr;

llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp

 //===- lib/Transforms/Utils/FunctionImportUtils.cpp - Importing utilities -===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the FunctionImportGlobalProcessing class, used
 // to perform the necessary global value handling for function importing.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
+#include "llvm/IR/InstIterator.h"
+#include "llvm/IR/Instructions.h"
 using namespace llvm;
 
 /// Checks if we should import SGV as a definition, otherwise import as a
 /// declaration.
 bool FunctionImportGlobalProcessing::doImportAsDefinition(
     const GlobalValue *SGV, DenseSet<const GlobalValue *> *GlobalsToImport) {
 
   // For alias, we tie the definition to the base object. Extract it and recurse
if (GO && GO->isDeclarationForLinker() && GO->hasComdat()) {
     // at this point would be a definition imported as available_externally.
     assert(GO->hasAvailableExternallyLinkage() &&
            "Expected comdat on definition (possibly available external)");
     GO->setComdat(nullptr);
   }
 }
 
 void FunctionImportGlobalProcessing::processGlobalsForThinLTO() {
-  // We cannot currently promote or rename anything that is in llvm.used,
-  // since any such value may have a use that won't see the new name.
-  // Specifically, any uses within inline assembly are not visible to the
-  // compiler. Prevent changing any such values on the exporting side,
+  // We cannot currently promote or rename anything used in inline assembly,
+  // which are not visible to the compiler. Detect a possible case by looking
+  // for a llvm.used local value, in conjunction with an inline assembly call
+  // in the module. Prevent changing any such values on the exporting side,
   // since we would already have guarded against an import from this module by
   // suppressing its index generation. See comments on what is required
   // in order to implement a finer grained solution in
   // ModuleSummaryIndexBuilder::ModuleSummaryIndexBuilder().
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
+  bool LocalIsUsed = false;
   for (GlobalValue *V : Used) {
-    if (!V->hasLocalLinkage())
-      continue;
     // We would have blocked importing from this module by suppressing index
     // generation.
-    assert(!isPerformingImport() &&
+    assert((!V->hasLocalLinkage() || !isPerformingImport()) &&
            "Should have blocked importing from module with local used");
-    return;
+    if ((LocalIsUsed |= V->hasLocalLinkage()))
+      break;
   }
+  if (LocalIsUsed)
+    for (auto &F : M)
+      for (auto &I : instructions(F))
+        if (const CallInst *CallI = dyn_cast<CallInst>(&I))
+          if (CallI->isInlineAsm())
+            return;
 
   for (GlobalVariable &GV : M.globals())
     processGlobalForThinLTO(GV);
   for (Function &SF : M)
     processGlobalForThinLTO(SF);
   for (GlobalAlias &GA : M.aliases())
     processGlobalForThinLTO(GA);
 }