ThinLTO: fix assertion and refactor check for hidden use from inline ASM in a helper function

[mehdi_amini]

Summary

This test was crashing, and currently it breaks bootstrapping clang with debuginfo

[mehdi_amini]

Sorry i didn't add the test: but you should figure out what the fix is about: a module with an internal function in LLVM.used but without any inline ASM in the module.

[tejohnson]

LGTM with a test added and nit fixed.

lib/Transforms/Utils/FunctionImportUtils.cpp

230

Oh I see, hitting this assert I guess.

244

Extraneous new line added.

[mehdi_amini]

Add test and remove spurious new line

[mehdi_amini]

lib/Transforms/Utils/FunctionImportUtils.cpp

230

Yes!

[mehdi_amini]

r268715

include/llvm/Analysis/ModuleSummaryAnalysis.h

 };
 
 //===--------------------------------------------------------------------===//
 //
 // createModuleSummaryIndexWrapperPass - This pass builds a ModuleSummaryIndex
 // object for the module, to be written to bitcode or LLVM assembly.
 //
 ModulePass *createModuleSummaryIndexWrapperPass();
+
+/// Returns true if \p M is eligible for ThinLTO promotion.
+///
+/// Currently we check if it has any any InlineASM that uses an internal symbol.
+bool moduleCanBeRenamedForThinLTO(const Module &M);
 }
 
 #endif

lib/Analysis/ModuleSummaryAnalysis.cpp

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 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 ((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;
+  // Check if the module can be promoted, otherwise just disable importing from
+  // it by not emitting any summary.
+  // FIXME: we could still import *into* it most of the time.
+  if (!moduleCanBeRenamedForThinLTO(*M))
 
   // Compute summaries for all functions defined in module, and save in the
   // index.
   for (auto &F : *M) {
     if (F.isDeclaration())
       continue;
 
     BlockFrequencyInfo *BFI = nullptr;
bool ModuleSummaryIndexWrapperPass::doFinalization(Module &M) {
   IndexBuilder.reset();
   return false;
 }
 
 void ModuleSummaryIndexWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesAll();
   AU.addRequired<BlockFrequencyInfoWrapperPass>();
 }
+
+bool llvm::moduleCanBeRenamedForThinLTO(const Module &M) {
+  // 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 =
+      llvm::any_of(Used, [](GlobalValue *V) { return V->hasLocalLinkage(); });
+  if (!LocalIsUsed)
+    return true;
+
+  // Walk all the instructions in the module and find if one is inline ASM
+  auto HasInlineAsm = llvm::any_of(M, [](const Function &F) {
+    return llvm::any_of(instructions(F), [](const Instruction &I) {
+      const CallInst *CallI = dyn_cast<CallInst>(&I);
+      if (!CallI)
+        return false;
+      return CallI->isInlineAsm();
+    });
+  });
+  return !HasInlineAsm;
+}

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/Analysis/ModuleSummaryAnalysis.h"
 #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(
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 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 (!moduleCanBeRenamedForThinLTO(M)) {
     // We would have blocked importing from this module by suppressing index
-    // generation.
-    assert((!V->hasLocalLinkage() || !isPerformingImport()) &&
-           "Should have blocked importing from module with local used");
-    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;
+    // generation. We still may be able to import into this module though.
+    assert(!isPerformingImport() &&
+           "Should have blocked importing from module with local used in ASM");
+  }
 
   for (GlobalVariable &GV : M.globals())
     processGlobalForThinLTO(GV);
   for (Function &SF : M)
     processGlobalForThinLTO(SF);
   for (GlobalAlias &GA : M.aliases())
     processGlobalForThinLTO(GA);
 }
 
 bool FunctionImportGlobalProcessing::run() {
   processGlobalsForThinLTO();
   return false;
 }
 
 bool llvm::renameModuleForThinLTO(
     Module &M, const ModuleSummaryIndex &Index,
     DenseSet<const GlobalValue *> *GlobalsToImport) {
   FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport);
   return ThinLTOProcessing.run();
 }

test/ThinLTO/X86/Inputs/llvm.used.ll

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+
+define i32 @main() #0 {
+entry:
+  call void (...) @globalfunc()
+  ret i32 0
+}
+
+declare void @globalfunc(...)
 No newline at end of file

test/ThinLTO/X86/llvm.used.ll

+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary %s -o %t.bc
+; RUN: opt -module-summary %p/Inputs/llvm.used.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
+
+
+; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s
+; CHECK: define available_externally void @globalfunc
+
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+
+
+define internal void @_ZN12_GLOBAL__N_16Module4dumpEv() {
+    ret void
+}
[e-mail] = appending global [1 x i8*] [i8* bitcast (void ()* @_ZN12_GLOBAL__N_16Module4dumpEv to i8*)], section "llvm.metadata"
+
+
+define void @globalfunc() #0 {
+entry:
+  ret void
+}