FunctionImport: Use IRMover directly. NFCI.

[pcc]

Summary

The importer was previously using ModuleLinker in a sort of "IRMover mode". Use
IRMover directly instead in order to remove a level of indirection.

I will remove all importing support from ModuleLinker in a separate
change.

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

724 ↗

(On Diff #86868)

I guess you have to do this to make it NFC, since it looks like we were doing this in the ModuleLinker.
Interesting...I didn't realize or had forgotten that the ModuleLinker would do this - so essentially we always import an alias if its aliasee is linkonce_odr and marked for import? That seems like something we want to get rid of (can be in a follow-up change so this remains NFC). We've tried to get to the point where we only import values decided on by the thin link. This is presumably a holdover from the old scheme of always importing linkonce_odr references...

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

28 ↗

(On Diff #86868)

This looks like a bug fix?

[pcc]

• Add comment

llvm/lib/Transforms/IPO/FunctionImport.cpp

724 ↗

(On Diff #86868)

Yes, this was needed in order to be NFC. I've added a FIXME to remove.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

28 ↗

(On Diff #86868)

Yes, and also needed in order to be NFC. Without this we would import @linkonceODRfuncLinkonceAlias in test/ThinLTO/X86/alias_import.ll because the logic for whether to put globals in GlobalsToImport differed from that in FunctionImportGlobalProcessing::doImportAsDefinition and we would normally only import globals that satisfied both (this is because we were querying the latter on LinkModules.cpp:394 in the old code and querying the former on line 277).

But naturally that raises the question of why the change on line FunctionImport.cpp:724 is needed at all. The answer is that we only call shouldLinkFromSource if the destination defines a GV with the same name, so if the destination did not define the GV we would only test FunctionImportGlobalProcessing::doImportAsDefinition.

In short: ModuleLinker is complicated, and it's definitely a Good Thing that we're moving away from it.

[mehdi_amini]

LGTM, but please wait for Teresa :)

llvm/lib/Transforms/IPO/FunctionImport.cpp

724 ↗

(On Diff #86868)

I didn't realize or had forgotten that the ModuleLinker would do this

This is exactly what always bothered me with us using the ModuleLinker instead of the IRMover :)

[tejohnson]

LGTM but please remove the NFC from the description, due to the isInterposable change and the comdat handling difference noted on IRC. I did build SPEC with this change and it isn't causing any failures (there is a failure to link soplex due to hidden symbols, but it is still there without this patch and likely due to D28978).

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

class FunctionImportGlobalProcessing {
   /// The Module which we are exporting or importing functions from.
   Module &M;
 
   /// Module summary index passed in for function importing/exporting handling.
   const ModuleSummaryIndex &ImportIndex;
 
   /// Globals to import from this module, all other functions will be
   /// imported as declarations instead of definitions.
-  DenseSet<const GlobalValue *> *GlobalsToImport;
+  SetVector<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;
 
   /// Set of llvm.*used values, in order to validate that we don't try
#endif
   /// module. Specifically, for ThinLTO importing or exporting it may need
   /// to be adjusted. When \p DoPromote is true then we must adjust the
   /// linkage for a required promotion of a local to global scope.
   GlobalValue::LinkageTypes getLinkage(const GlobalValue *SGV, bool DoPromote);
 
 public:
   FunctionImportGlobalProcessing(
       Module &M, const ModuleSummaryIndex &Index,
-      DenseSet<const GlobalValue *> *GlobalsToImport = nullptr)
+      SetVector<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 (!GlobalsToImport)
       HasExportedFunctions = ImportIndex.hasExportedFunctions(M);
 
 #ifndef NDEBUG
     // First collect those in the llvm.used set.
     collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
     // Next collect those in the llvm.compiler.used set.
     collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true);
 #endif
   }
 
   bool run();
 
-  static bool
-  doImportAsDefinition(const GlobalValue *SGV,
-                       DenseSet<const GlobalValue *> *GlobalsToImport);
+  static bool doImportAsDefinition(const GlobalValue *SGV,
+                                   SetVector<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,
-    DenseSet<const GlobalValue *> *GlobalsToImport = nullptr);
+    SetVector<GlobalValue *> *GlobalsToImport = nullptr);
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/Linker/LinkModules.cpp

public:
 
   bool run();
 };
 }
 
 bool ModuleLinker::doImportAsDefinition(const GlobalValue *SGV) {
   if (!isPerformingImport())
     return false;
-  return FunctionImportGlobalProcessing::doImportAsDefinition(SGV,
-                                                              GlobalsToImport);
+  report_fatal_error("ModuleLinker does not support importing");
 }
 
 static GlobalValue::VisibilityTypes
 getMinVisibility(GlobalValue::VisibilityTypes A,
                  GlobalValue::VisibilityTypes B) {
   if (A == GlobalValue::HiddenVisibility || B == GlobalValue::HiddenVisibility)
     return GlobalValue::HiddenVisibility;
   if (A == GlobalValue::ProtectedVisibility ||

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

 // index.
 //
 Expected<bool> FunctionImporter::importFunctions(
     Module &DestModule, const FunctionImporter::ImportMapTy &ImportList) {
   DEBUG(dbgs() << "Starting import for Module "
                << DestModule.getModuleIdentifier() << "\n");
   unsigned ImportedCount = 0;
 
-  // Linker that will be used for importing function
-  Linker TheLinker(DestModule);
+  IRMover Mover(DestModule);
   // Do the actual import of functions now, one Module at a time
   std::set<StringRef> ModuleNameOrderedList;
   for (auto &FunctionsToImportPerModule : ImportList) {
     ModuleNameOrderedList.insert(FunctionsToImportPerModule.first());
   }
   for (auto &Name : ModuleNameOrderedList) {
     // Get the module for the import
     const auto &FunctionsToImportPerModule = ImportList.find(Name);
     assert(FunctionsToImportPerModule != ImportList.end());
     Expected<std::unique_ptr<Module>> SrcModuleOrErr = ModuleLoader(Name);
     if (!SrcModuleOrErr)
       return SrcModuleOrErr.takeError();
     std::unique_ptr<Module> SrcModule = std::move(*SrcModuleOrErr);
     assert(&DestModule.getContext() == &SrcModule->getContext() &&
            "Context mismatch");
 
     // If modules were created with lazy metadata loading, materialize it
     // now, before linking it (otherwise this will be a noop).
     if (Error Err = SrcModule->materializeMetadata())
       return std::move(Err);
 
     auto &ImportGUIDs = FunctionsToImportPerModule->second;
     // Find the globals to import
-    DenseSet<const GlobalValue *> GlobalsToImport;
+    SetVector<GlobalValue *> GlobalsToImport;
     for (Function &F : *SrcModule) {
       if (!F.hasName())
         continue;
       auto GUID = F.getGUID();
       auto Import = ImportGUIDs.count(GUID);
       DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing function " << GUID
                    << " " << F.getName() << " from "
                    << SrcModule->getSourceFileName() << "\n");
for (GlobalVariable &GV : SrcModule->globals()) {
                    << SrcModule->getSourceFileName() << "\n");
       if (Import) {
         if (Error Err = GV.materialize())
           return std::move(Err);
         GlobalsToImport.insert(&GV);
       }
     }
     for (GlobalAlias &GA : SrcModule->aliases()) {
+      // FIXME: This should eventually be controlled entirely by the summary.
+      if (FunctionImportGlobalProcessing::doImportAsDefinition(
+              &GA, &GlobalsToImport)) {
+        GlobalsToImport.insert(&GA);
+        continue;
+      }
+
       if (!GA.hasName())
         continue;
       auto GUID = GA.getGUID();
       auto Import = ImportGUIDs.count(GUID);
       DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing alias " << GUID
                    << " " << GA.getName() << " from "
                    << SrcModule->getSourceFileName() << "\n");
       if (Import) {
if (renameModuleForThinLTO(*SrcModule, Index, &GlobalsToImport))
       return true;
 
     if (PrintImports) {
       for (const auto *GV : GlobalsToImport)
         dbgs() << DestModule.getSourceFileName() << ": Import " << GV->getName()
                << " from " << SrcModule->getSourceFileName() << "\n";
     }
 
-    // Instruct the linker that the client will take care of linkonce resolution
-    unsigned Flags = Linker::Flags::DontForceLinkLinkonceODR;
-
-    if (TheLinker.linkInModule(std::move(SrcModule), Flags, &GlobalsToImport))
+    if (Mover.move(std::move(SrcModule), GlobalsToImport.getArrayRef(),
+                   [](GlobalValue &, IRMover::ValueAdder) {},
+                   /*LinkModuleInlineAsm=*/false, /*IsPerformingImport=*/true))
       report_fatal_error("Function Import: link error");
 
     ImportedCount += GlobalsToImport.size();
     NumImportedModules++;
   }
 
   NumImportedFunctions += ImportedCount;
 

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

 #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) {
+    const GlobalValue *SGV, SetVector<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())
+    if (GA->isInterposable())
       return false;
     const GlobalObject *GO = GA->getBaseObject();
     if (!GO->hasLinkOnceODRLinkage())
       return false;
     return FunctionImportGlobalProcessing::doImportAsDefinition(
         GO, GlobalsToImport);
   }
   // Only import the globals requested for importing.
-  if (GlobalsToImport->count(SGV))
+  if (GlobalsToImport->count(const_cast<GlobalValue *>(SGV)))
     return true;
   // Otherwise no.
   return false;
 }
 
 bool FunctionImportGlobalProcessing::doImportAsDefinition(
     const GlobalValue *SGV) {
   if (!isPerformingImport())
     return false;
   return FunctionImportGlobalProcessing::doImportAsDefinition(SGV,
                                                               GlobalsToImport);
 }
 
 bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(
     const GlobalValue *SGV) {
   assert(SGV->hasLocalLinkage());
   // Both the imported references and the original local variable must
   // be promoted.
   if (!isPerformingImport() && !isModuleExporting())
     return false;
 
   if (isPerformingImport()) {
-    assert((!GlobalsToImport->count(SGV) || !isNonRenamableLocal(*SGV)) &&
+    assert((!GlobalsToImport->count(const_cast<GlobalValue *>(SGV)) ||
+            !isNonRenamableLocal(*SGV)) &&
            "Attempting to promote non-renamable local");
     // We don't know for sure yet if we are importing this value (as either
     // a reference or a def), since we are simply walking all values in the
     // module. But by necessity if we end up importing it and it is local,
     // it must be promoted, so unconditionally promote all values in the
     // importing module.
     return true;
   }
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) {
+bool llvm::renameModuleForThinLTO(Module &M, const ModuleSummaryIndex &Index,
+                                  SetVector<GlobalValue *> *GlobalsToImport) {
   FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport);
   return ThinLTOProcessing.run();
 }