Rework linkInModule(), making it oblivious to ThinLTO

Summary:
ThinLTO is relying on linkInModule to import selected function.
However a lot of "magic" was hidden in linkInModule and the IRMover,
who would rename and promote global variables on the fly.

This is moving to an approach where the steps are decoupled and the
client is reponsible to specify the list of globals to import.
As a consequence some test are changed because they were relying on
the previous behavior which was importing the definition of *every*
single global without control on the client side.
Now the burden is on the client to decide if a global has to be imported
or not.

Reviewers: tejohnson

Subscribers: joker.eph, llvm-commits

Differential Revision: http://reviews.llvm.org/D18122

From: Mehdi Amini <mehdi.amini@apple.com>
llvm-svn: 263863

llvm/include/llvm/Linker/Linker.h

@@ -10,7 +10,6 @@
 #ifndef LLVM_LINKER_LINKER_H
 #define LLVM_LINKER_LINKER_H
 
-#include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/Linker/IRMover.h"
 
 namespace llvm {
@@ -40,15 +39,14 @@ class Linker {
   /// Passing OverrideSymbols as true will have symbols from Src
   /// shadow those in the Dest.
   /// For ThinLTO function importing/exporting the \p ModuleSummaryIndex
-  /// is passed. If \p FunctionsToImport is provided, only the functions that
+  /// is passed. If \p GlobalsToImport is provided, only the globals that
   /// are part of the set will be imported from the source module.
   /// The \p ValIDToTempMDMap is populated by the linker when function
   /// importing is performed.
   ///
   /// Returns true on error.
   bool linkInModule(std::unique_ptr<Module> Src, unsigned Flags = Flags::None,
-                    const ModuleSummaryIndex *Index = nullptr,
-                    DenseSet<const GlobalValue *> *FunctionsToImport = nullptr,
+                    DenseSet<const GlobalValue *> *GlobalsToImport = nullptr,
                     DenseMap<unsigned, MDNode *> *ValIDToTempMDMap = nullptr);
 
   static bool linkModules(Module &Dest, std::unique_ptr<Module> Src,

llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h

@@ -30,27 +30,22 @@ class FunctionImportGlobalProcessing {
   /// Module summary index passed in for function importing/exporting handling.
   const ModuleSummaryIndex &ImportIndex;
 
-  /// Functions to import from this module, all other functions will be
+  /// Globals to import from this module, all other functions will be
   /// imported as declarations instead of definitions.
-  DenseSet<const GlobalValue *> *FunctionsToImport;
+  DenseSet<const GlobalValue *> *GlobalsToImport;
 
   /// Set to true if the given ModuleSummaryIndex contains any functions
   /// from this source module, in which case we must conservatively assume
   /// that any of its functions may be imported into another module
   /// as part of a different backend compilation process.
   bool HasExportedFunctions = false;
 
-  /// Populated during ThinLTO global processing with locals promoted
-  /// to global scope in an exporting module, which now need to be linked
-  /// in if calling from the ModuleLinker.
-  SetVector<GlobalValue *> NewExportedValues;
-
   /// Check if we should promote the given local value to global scope.
   bool doPromoteLocalToGlobal(const GlobalValue *SGV);
 
   /// Helper methods to check if we are importing from or potentially
   /// exporting from the current source module.
-  bool isPerformingImport() const { return FunctionsToImport != nullptr; }
+  bool isPerformingImport() const { return GlobalsToImport != nullptr; }
   bool isModuleExporting() const { return HasExportedFunctions; }
 
   /// If we are importing from the source module, checks if we should
@@ -77,29 +72,28 @@ class FunctionImportGlobalProcessing {
 public:
   FunctionImportGlobalProcessing(
       Module &M, const ModuleSummaryIndex &Index,
-      DenseSet<const GlobalValue *> *FunctionsToImport = nullptr)
-      : M(M), ImportIndex(Index), FunctionsToImport(FunctionsToImport) {
+      DenseSet<const GlobalValue *> *GlobalsToImport = nullptr)
+      : M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport) {
     // If we have a ModuleSummaryIndex but no function to import,
     // then this is the primary module being compiled in a ThinLTO
     // backend compilation, and we need to see if it has functions that
     // may be exported to another backend compilation.
-    if (!FunctionsToImport)
+    if (!GlobalsToImport)
       HasExportedFunctions = ImportIndex.hasExportedFunctions(M);
   }
 
   bool run();
 
   static bool
   doImportAsDefinition(const GlobalValue *SGV,
-                       DenseSet<const GlobalValue *> *FunctionsToImport);
-
-  /// Access the promoted globals that are now exported and need to be linked.
-  SetVector<GlobalValue *> &getNewExportedValues() { return NewExportedValues; }
+                       DenseSet<const GlobalValue *> *GlobalsToImport);
 };
 
 /// Perform in-place global value handling on the given Module for
 /// exported local functions renamed and promoted for ThinLTO.
-bool renameModuleForThinLTO(Module &M, const ModuleSummaryIndex &Index);
+bool renameModuleForThinLTO(
+    Module &M, const ModuleSummaryIndex &Index,
+    DenseSet<const GlobalValue *> *GlobalsToImport = nullptr);
 
 } // End llvm namespace
 

llvm/lib/Linker/LinkModules.cpp

@@ -35,19 +35,9 @@ class ModuleLinker {
   /// For symbol clashes, prefer those from Src.
   unsigned Flags;
 
-  /// Module summary index passed into ModuleLinker for using in function
-  /// importing/exporting handling.
-  const ModuleSummaryIndex *ImportIndex;
-
   /// Functions to import from source module, all other functions are
   /// imported as declarations instead of definitions.
-  DenseSet<const GlobalValue *> *FunctionsToImport;
-
-  /// Set to true if the given ModuleSummaryIndex contains any functions
-  /// from this source module, in which case we must conservatively assume
-  /// that any of its functions may be imported into another module
-  /// as part of a different backend compilation process.
-  bool HasExportedFunctions = false;
+  DenseSet<const GlobalValue *> *GlobalsToImport;
 
   /// Association between metadata value id and temporary metadata that
   /// remains unmapped after function importing. Saved during function
@@ -116,29 +106,18 @@ class ModuleLinker {
 
   /// Helper method to check if we are importing from the current source
   /// module.
-  bool isPerformingImport() const { return FunctionsToImport != nullptr; }
+  bool isPerformingImport() const { return GlobalsToImport != nullptr; }
 
   /// If we are importing from the source module, checks if we should
   /// import SGV as a definition, otherwise import as a declaration.
   bool doImportAsDefinition(const GlobalValue *SGV);
 
 public:
   ModuleLinker(IRMover &Mover, std::unique_ptr<Module> SrcM, unsigned Flags,
-               const ModuleSummaryIndex *Index = nullptr,
-               DenseSet<const GlobalValue *> *FunctionsToImport = nullptr,
+               DenseSet<const GlobalValue *> *GlobalsToImport = nullptr,
                DenseMap<unsigned, MDNode *> *ValIDToTempMDMap = nullptr)
-      : Mover(Mover), SrcM(std::move(SrcM)), Flags(Flags), ImportIndex(Index),
-        FunctionsToImport(FunctionsToImport),
-        ValIDToTempMDMap(ValIDToTempMDMap) {
-    assert((ImportIndex || !FunctionsToImport) &&
-           "Expect a ModuleSummaryIndex when importing");
-    // If we have a ModuleSummaryIndex but no function to import,
-    // then this is the primary module being compiled in a ThinLTO
-    // backend compilation, and we need to see if it has functions that
-    // may be exported to another backend compilation.
-    if (ImportIndex && !FunctionsToImport)
-      HasExportedFunctions = ImportIndex->hasExportedFunctions(*this->SrcM);
-  }
+      : Mover(Mover), SrcM(std::move(SrcM)), Flags(Flags),
+        GlobalsToImport(GlobalsToImport), ValIDToTempMDMap(ValIDToTempMDMap) {}
 
   bool run();
 };
@@ -147,8 +126,8 @@ class ModuleLinker {
 bool ModuleLinker::doImportAsDefinition(const GlobalValue *SGV) {
   if (!isPerformingImport())
     return false;
-  return FunctionImportGlobalProcessing::doImportAsDefinition(
-      SGV, FunctionsToImport);
+  return FunctionImportGlobalProcessing::doImportAsDefinition(SGV,
+                                                              GlobalsToImport);
 }
 
 static GlobalValue::VisibilityTypes
@@ -297,7 +276,7 @@ bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
     if (isa<Function>(&Src)) {
       // For functions, LinkFromSrc iff this is a function requested
       // for importing. For variables, decide below normally.
-      LinkFromSrc = FunctionsToImport->count(&Src);
+      LinkFromSrc = GlobalsToImport->count(&Src);
       return false;
     }
 
@@ -423,12 +402,12 @@ bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
   if (GV.hasAppendingLinkage() && isPerformingImport())
     return false;
 
-  if (isPerformingImport() && !doImportAsDefinition(&GV))
-    return false;
-
-  if (!DGV && !shouldOverrideFromSrc() &&
-      (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||
-       GV.hasAvailableExternallyLinkage()))
+  if (isPerformingImport()) {
+    if (!doImportAsDefinition(&GV))
+      return false;
+  } else if (!DGV && !shouldOverrideFromSrc() &&
+             (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||
+              GV.hasAvailableExternallyLinkage()))
     return false;
 
   if (GV.isDeclaration())
@@ -508,15 +487,6 @@ bool ModuleLinker::run() {
     if (linkIfNeeded(GA))
       return true;
 
-  if (ImportIndex) {
-    FunctionImportGlobalProcessing ThinLTOProcessing(*SrcM, *ImportIndex,
-                                                     FunctionsToImport);
-    if (ThinLTOProcessing.run())
-      return true;
-    for (auto *GV : ThinLTOProcessing.getNewExportedValues())
-      ValuesToLink.insert(GV);
-  }
-
   for (unsigned I = 0; I < ValuesToLink.size(); ++I) {
     GlobalValue *GV = ValuesToLink[I];
     const Comdat *SC = GV->getComdat();
@@ -549,10 +519,9 @@ bool ModuleLinker::run() {
 Linker::Linker(Module &M) : Mover(M) {}
 
 bool Linker::linkInModule(std::unique_ptr<Module> Src, unsigned Flags,
-                          const ModuleSummaryIndex *Index,
-                          DenseSet<const GlobalValue *> *FunctionsToImport,
+                          DenseSet<const GlobalValue *> *GlobalsToImport,
                           DenseMap<unsigned, MDNode *> *ValIDToTempMDMap) {
-  ModuleLinker ModLinker(Mover, std::move(Src), Flags, Index, FunctionsToImport,
+  ModuleLinker ModLinker(Mover, std::move(Src), Flags, GlobalsToImport,
                          ValIDToTempMDMap);
   return ModLinker.run();
 }

llvm/lib/Transforms/IPO/FunctionImport.cpp

@@ -266,7 +266,6 @@ GetImportList(Module &DestModule,
     if (!F && isa<GlobalAlias>(SGV)) {
       auto *SGA = dyn_cast<GlobalAlias>(SGV);
       F = dyn_cast<Function>(SGA->getBaseObject());
-      CalledFunctionName = F->getName();
     }
     assert(F && "Imported Function is ... not a Function");
 
@@ -349,8 +348,11 @@ bool FunctionImporter::importFunctions(Module &DestModule) {
     UpgradeDebugInfo(*SrcModule);
 
     // Link in the specified functions.
+    if (renameModuleForThinLTO(*SrcModule, Index, &FunctionsToImport))
+      return true;
+
     if (TheLinker.linkInModule(std::move(SrcModule), Linker::Flags::None,
-                               &Index, &FunctionsToImport))
+                               &FunctionsToImport))
       report_fatal_error("Function Import: link error");
 
     ImportedCount += FunctionsToImport.size();

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

@@ -18,30 +18,20 @@ 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 *> *FunctionsToImport) {
-  auto *GA = dyn_cast<GlobalAlias>(SGV);
-  if (GA) {
+    const GlobalValue *SGV, DenseSet<const GlobalValue *> *GlobalsToImport) {
+
+  // For alias, we tie the definition to the base object. Extract it and recurse
+  if (auto *GA = dyn_cast<GlobalAlias>(SGV)) {
     if (GA->hasWeakAnyLinkage())
       return false;
     const GlobalObject *GO = GA->getBaseObject();
     if (!GO->hasLinkOnceODRLinkage())
       return false;
     return FunctionImportGlobalProcessing::doImportAsDefinition(
-        GO, FunctionsToImport);
+        GO, GlobalsToImport);
   }
-  // Always import GlobalVariable definitions, except for the special
-  // case of WeakAny which are imported as ExternalWeak declarations
-  // (see comments in FunctionImportGlobalProcessing::getLinkage). The linkage
-  // changes described in FunctionImportGlobalProcessing::getLinkage ensure the
-  // correct behavior (e.g. global variables with external linkage are
-  // transformed to available_externally definitions, which are ultimately
-  // turned into declarations after the EliminateAvailableExternally pass).
-  if (isa<GlobalVariable>(SGV) && !SGV->isDeclaration() &&
-      !SGV->hasWeakAnyLinkage())
-    return true;
-  // Only import the function requested for importing.
-  auto *SF = dyn_cast<Function>(SGV);
-  if (SF && FunctionsToImport->count(SF))
+  // Only import the globals requested for importing.
+  if (GlobalsToImport->count(SGV))
     return true;
   // Otherwise no.
   return false;
@@ -51,8 +41,8 @@ bool FunctionImportGlobalProcessing::doImportAsDefinition(
     const GlobalValue *SGV) {
   if (!isPerformingImport())
     return false;
-  return FunctionImportGlobalProcessing::doImportAsDefinition(
-      SGV, FunctionsToImport);
+  return FunctionImportGlobalProcessing::doImportAsDefinition(SGV,
+                                                              GlobalsToImport);
 }
 
 bool FunctionImportGlobalProcessing::doPromoteLocalToGlobal(
@@ -198,8 +188,6 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
     GV.setLinkage(getLinkage(&GV));
     if (!GV.hasLocalLinkage())
       GV.setVisibility(GlobalValue::HiddenVisibility);
-    if (isModuleExporting())
-      NewExportedValues.insert(&GV);
   } else
     GV.setLinkage(getLinkage(&GV));
 
@@ -231,7 +219,9 @@ bool FunctionImportGlobalProcessing::run() {
   return false;
 }
 
-bool llvm::renameModuleForThinLTO(Module &M, const ModuleSummaryIndex &Index) {
-  FunctionImportGlobalProcessing ThinLTOProcessing(M, Index);
+bool llvm::renameModuleForThinLTO(
+    Module &M, const ModuleSummaryIndex &Index,
+    DenseSet<const GlobalValue *> *GlobalsToImport) {
+  FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport);
   return ThinLTOProcessing.run();
 }

llvm/test/Linker/funcimport.ll

@@ -67,7 +67,7 @@
 ; Ensure that imported static variable and function references are correctly
 ; promoted and renamed (including static constant variable).
 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=referencestatics:%t.bc -S | FileCheck %s --check-prefix=IMPORTSTATIC
-; IMPORTSTATIC-DAG: @staticvar.llvm.1 = available_externally hidden global
+; IMPORTSTATIC-DAG: @staticvar.llvm.1 = external hidden global
 ; IMPORTSTATIC-DAG: @staticconstvar.llvm.1 = internal unnamed_addr constant
 ; IMPORTSTATIC-DAG: define available_externally i32 @referencestatics
 ; IMPORTSTATIC-DAG: %call = call i32 @staticfunc.llvm.1
@@ -78,18 +78,18 @@
 ; are handled correctly (including referenced variable imported as
 ; available_externally definition)
 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=referenceglobals:%t.bc -S | FileCheck %s --check-prefix=IMPORTGLOBALS
-; IMPORTGLOBALS-DAG: @globalvar = available_externally global
+; IMPORTGLOBALS-DAG: @globalvar = external global
 ; IMPORTGLOBALS-DAG: declare void @globalfunc1()
 ; IMPORTGLOBALS-DAG: define available_externally i32 @referenceglobals
 
 ; Ensure that common variable correctly imported as common defition.
 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=referencecommon:%t.bc -S | FileCheck %s --check-prefix=IMPORTCOMMON
-; IMPORTCOMMON-DAG: @commonvar = common global
+; IMPORTCOMMON-DAG: @commonvar = external global
 ; IMPORTCOMMON-DAG: define available_externally i32 @referencecommon
 
 ; Ensure that imported static function pointer correctly promoted and renamed.
 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=callfuncptr:%t.bc -S | FileCheck %s --check-prefix=IMPORTFUNCPTR
-; IMPORTFUNCPTR-DAG: @P.llvm.1 = available_externally hidden global void ()* null
+; IMPORTFUNCPTR-DAG: @P.llvm.1 = external hidden global void ()*
 ; IMPORTFUNCPTR-DAG: define available_externally void @callfuncptr
 ; IMPORTFUNCPTR-DAG: %0 = load void ()*, void ()** @P.llvm.1
 

llvm/test/Transforms/FunctionImport/funcimport.ll

@@ -65,7 +65,7 @@ declare void @callfuncptr(...) #1
 
 ; Ensure that all uses of local variable @P which has used in setfuncptr
 ; and callfuncptr are to the same promoted/renamed global.
-; CHECK-DAG: @P.llvm.2 = available_externally hidden global void ()* null
+; CHECK-DAG: @P.llvm.2 = external hidden global void ()*
 ; CHECK-DAG: %0 = load void ()*, void ()** @P.llvm.2,
 ; CHECK-DAG: store void ()* @staticfunc2.llvm.2, void ()** @P.llvm.2,