[ThinLTO] Enable in-place symbol changes for exporting module

[tejohnson]

Summary

Move ThinLTO global value processing functions out of ModuleLinker and
into a new ThinLTOGlobalProcessor class, which performs any necessary
linkage and naming changes on the given module in place.

As a result, renameModuleForThinLTO no longer needs to create a new
Module when performing any necessary local to global promotion on a
module that we are possibly exporting from during a ThinLTO backend
compilation.

During function importing the ThinLTO processing is still invoked from
the ModuleLinker (via the new class), as it needs to perform renaming and
linkage changes on the source module, e.g. in order to get the correct
renaming during local to global promotion.

[tejohnson]

Once this goes in, I can change the signature of renameModuleForThinLTO so that it takes a Module reference and returns a bool instead of a unique_ptr, and then move the call out of clang (and gold-plugin in D15390) and into FunctionImportPass.

Note doing the ThinLTO promotion/renaming in a single pass over the module after function importing is done seems very difficult, for a couple reasons. One is that we need the source module information to determine how to rename during promotion. It is much easier to invoke on each source module being imported from. Also, to support ThinLTO testing via llvm-link, which does not run the pass manager.

[mehdi_amini]

Thanks for this patch, I was complaining about the ModuleLinker doing to much stuff in a review a few weeks ago, and splitting it / extracting pieces is definitively something valuable.

lib/Linker/LinkModules.cpp

130 ↗

(On Diff #43383)

clang-format

191 ↗

(On Diff #43383)

Why was this declaration moved? Keeping it after processGlobalForThinLTO allows for a smaller diff.

[tejohnson]

lib/Linker/LinkModules.cpp

130 ↗

(On Diff #43383)

I did (just tried again) and it doesn't touch this. What looks wrong?

191 ↗

(On Diff #43383)

I think I removed this then put it back and inadvertently moved it. Will put it back.

[tejohnson]

• Move getLinkage declaration back to original location.

[mehdi_amini]

lib/Linker/LinkModules.cpp

130 ↗

(On Diff #44019)

isn't it over 80 cols?

[tejohnson]

lib/Linker/LinkModules.cpp

130 ↗

(On Diff #44019)

Uh, yes it is. Ok, got git clang-format to fix this. Looks like I need to specify the file name along with the commit ids to get it to format unmodified files.

[tejohnson]

• Clang format.

[mehdi_amini]

If you just committed, git clang-format HEAD~ should do it for every file modified by the commit. In general it take a range, so it you provide the commit hash and the commit hash matches HEAD, you're giving it an empty range.

[mehdi_amini]

Anything blocking before committing this?

llvm/trunk/include/llvm/Linker/Linker.h

public:
   /// destroyed.
   ///
   /// The \p ValIDToTempMDMap sound have been populated earlier during function
   /// importing from \p Src.
   bool linkInMetadata(Module &Src,
                       DenseMap<unsigned, MDNode *> *ValIDToTempMDMap);
 };
 
-/// Create a new module with exported local functions renamed and promoted
-/// for ThinLTO.
+/// Perform in-place global value handling on the given Module for
+/// exported local functions renamed and promoted for ThinLTO.
 std::unique_ptr<Module> renameModuleForThinLTO(std::unique_ptr<Module> M,
                                                const FunctionInfoIndex *Index);
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/Linker/LinkModules.cpp

class ModuleLinker {
   void addLazyFor(GlobalValue &GV, IRMover::ValueAdder Add);
 
   bool shouldOverrideFromSrc() { return Flags & Linker::OverrideFromSrc; }
   bool shouldLinkOnlyNeeded() { return Flags & Linker::LinkOnlyNeeded; }
   bool shouldInternalizeLinkedSymbols() {
     return Flags & Linker::InternalizeLinkedSymbols;
   }
 
-  /// Check if we should promote the given local value to global scope.
-  bool doPromoteLocalToGlobal(const GlobalValue *SGV);
-
   bool shouldLinkFromSource(bool &LinkFromSrc, const GlobalValue &Dest,
                             const GlobalValue &Src);
 
   /// Should we have mover and linker error diag info?
   bool emitError(const Twine &Message) {
     SrcM.getContext().diagnose(LinkDiagnosticInfo(DS_Error, Message));
     return true;
   }
class ModuleLinker {
   DenseMap<const Comdat *, std::vector<GlobalValue *>> ComdatMembers;
 
   /// Given a global in the source module, return the global in the
   /// destination module that is being linked to, if any.
   GlobalValue *getLinkedToGlobal(const GlobalValue *SrcGV) {
     Module &DstM = Mover.getModule();
     // If the source has no name it can't link.  If it has local linkage,
     // there is no name match-up going on.
-    if (!SrcGV->hasName() || GlobalValue::isLocalLinkage(getLinkage(SrcGV)))
+    if (!SrcGV->hasName() || GlobalValue::isLocalLinkage(SrcGV->getLinkage()))
       return nullptr;
 
     // Otherwise see if we have a match in the destination module's symtab.
-    GlobalValue *DGV = DstM.getNamedValue(getName(SrcGV));
+    GlobalValue *DGV = DstM.getNamedValue(SrcGV->getName());
     if (!DGV)
       return nullptr;
 
     // If we found a global with the same name in the dest module, but it has
     // internal linkage, we are really not doing any linkage here.
     if (DGV->hasLocalLinkage())
       return nullptr;
 
     // Otherwise, we do in fact link to the destination global.
     return DGV;
   }
 
   bool linkIfNeeded(GlobalValue &GV);
 
+  /// Helper method to check if we are importing from the current source
+  /// module.
+  bool isPerformingImport() const { return FunctionsToImport != 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, Module &SrcM, unsigned Flags,
+               const FunctionInfoIndex *Index = nullptr,
+               DenseSet<const GlobalValue *> *FunctionsToImport = nullptr,
+               DenseMap<unsigned, MDNode *> *ValIDToTempMDMap = nullptr)
+      : Mover(Mover), SrcM(SrcM), Flags(Flags), ImportIndex(Index),
+        FunctionsToImport(FunctionsToImport),
+        ValIDToTempMDMap(ValIDToTempMDMap) {
+    assert((ImportIndex || !FunctionsToImport) &&
+           "Expect a FunctionInfoIndex when importing");
+    // If we have a FunctionInfoIndex 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(SrcM);
+    assert((ValIDToTempMDMap || !FunctionsToImport) &&
+           "Function importing must provide a ValIDToTempMDMap");
+  }
+
+  bool run();
+};
+
+/// Class to handle necessary GlobalValue changes required by ThinLTO including
+/// linkage changes and any necessary renaming.
+class ThinLTOGlobalProcessing {
+  /// The Module which we are exporting or importing functions from.
+  Module &M;
+
+  /// Function index passed in for function importing/exporting handling.
+  const FunctionInfoIndex *ImportIndex;
+
+  /// Functions to import from this module, all other functions will be
+  /// imported as declarations instead of definitions.
+  DenseSet<const GlobalValue *> *FunctionsToImport;
+
+  /// Set to true if the given FunctionInfoIndex 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 isModuleExporting() const { return HasExportedFunctions; }
 
   /// 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);
class ThinLTOGlobalProcessing {
   void processGlobalForThinLTO(GlobalValue &GV);
 
   /// Get the new linkage for SGV that should be used in the linked destination
   /// module. Specifically, for ThinLTO importing or exporting it may need
   /// to be adjusted.
   GlobalValue::LinkageTypes getLinkage(const GlobalValue *SGV);
 
 public:
-  ModuleLinker(IRMover &Mover, Module &SrcM, unsigned Flags,
-               const FunctionInfoIndex *Index = nullptr,
-               DenseSet<const GlobalValue *> *FunctionsToImport = nullptr,
-               DenseMap<unsigned, MDNode *> *ValIDToTempMDMap = nullptr)
-      : Mover(Mover), SrcM(SrcM), Flags(Flags), ImportIndex(Index),
-        FunctionsToImport(FunctionsToImport),
-        ValIDToTempMDMap(ValIDToTempMDMap) {
-    assert((ImportIndex || !FunctionsToImport) &&
-           "Expect a FunctionInfoIndex when importing");
+  ThinLTOGlobalProcessing(
+      Module &M, const FunctionInfoIndex *Index,
+      DenseSet<const GlobalValue *> *FunctionsToImport = nullptr)
+      : M(M), ImportIndex(Index), FunctionsToImport(FunctionsToImport) {
     // If we have a FunctionInfoIndex 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(SrcM);
-    assert((ValIDToTempMDMap || !FunctionsToImport) &&
-           "Function importing must provide a ValIDToTempMDMap");
+    if (!FunctionsToImport)
+      HasExportedFunctions = ImportIndex->hasExportedFunctions(M);
   }
 
   bool run();
+
+  /// Access the promoted globals that are now exported and need to be linked.
+  SetVector<GlobalValue *> &getNewExportedValues() { return NewExportedValues; }
 };
 }
 
-bool ModuleLinker::doImportAsDefinition(const GlobalValue *SGV) {
-  if (!isPerformingImport())
-    return false;
+/// Checks if we should import SGV as a definition, otherwise import as a
+/// declaration.
+static bool
+doImportAsDefinitionImpl(const GlobalValue *SGV,
+                         DenseSet<const GlobalValue *> *FunctionsToImport) {
   auto *GA = dyn_cast<GlobalAlias>(SGV);
   if (GA) {
     if (GA->hasWeakAnyLinkage())
       return false;
     const GlobalObject *GO = GA->getBaseObject();
     if (!GO->hasLinkOnceODRLinkage())
       return false;
-    return doImportAsDefinition(GO);
+    return doImportAsDefinitionImpl(GO, FunctionsToImport);
   }
   // Always import GlobalVariable definitions, except for the special
   // case of WeakAny which are imported as ExternalWeak declarations
   // (see comments in ModuleLinker::getLinkage). The linkage changes
   // described in ModuleLinker::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))
     return true;
   // Otherwise no.
   return false;
 }
 
-bool ModuleLinker::doPromoteLocalToGlobal(const GlobalValue *SGV) {
+bool ThinLTOGlobalProcessing::doImportAsDefinition(const GlobalValue *SGV) {
+  if (!isPerformingImport())
+    return false;
+  return doImportAsDefinitionImpl(SGV, FunctionsToImport);
+}
+
+bool ModuleLinker::doImportAsDefinition(const GlobalValue *SGV) {
+  if (!isPerformingImport())
+    return false;
+  return doImportAsDefinitionImpl(SGV, FunctionsToImport);
+}
+
+bool ThinLTOGlobalProcessing::doPromoteLocalToGlobal(const GlobalValue *SGV) {
   assert(SGV->hasLocalLinkage());
   // Both the imported references and the original local variable must
   // be promoted.
   if (!isPerformingImport() && !isModuleExporting())
     return false;
 
   // Local const variables never need to be promoted unless they are address
   // taken. The imported uses can simply use the clone created in this module.
   // For now we are conservative in determining which variables are not
   // address taken by checking the unnamed addr flag. To be more aggressive,
   // the address taken information must be checked earlier during parsing
   // of the module and recorded in the function index for use when importing
   // from that module.
   auto *GVar = dyn_cast<GlobalVariable>(SGV);
   if (GVar && GVar->isConstant() && GVar->hasUnnamedAddr())
     return false;
 
   // Eventually we only need to promote functions in the exporting module that
   // are referenced by a potentially exported function (i.e. one that is in the
   // function index).
   return true;
 }
 
-std::string ModuleLinker::getName(const GlobalValue *SGV) {
+std::string ThinLTOGlobalProcessing::getName(const GlobalValue *SGV) {
   // For locals that must be promoted to global scope, ensure that
   // the promoted name uniquely identifies the copy in the original module,
   // using the ID assigned during combined index creation. When importing,
   // we rename all locals (not just those that are promoted) in order to
   // avoid naming conflicts between locals imported from different modules.
   if (SGV->hasLocalLinkage() &&
       (doPromoteLocalToGlobal(SGV) || isPerformingImport()))
     return FunctionInfoIndex::getGlobalNameForLocal(
         SGV->getName(),
         ImportIndex->getModuleId(SGV->getParent()->getModuleIdentifier()));
   return SGV->getName();
 }
 
-GlobalValue::LinkageTypes ModuleLinker::getLinkage(const GlobalValue *SGV) {
+GlobalValue::LinkageTypes
+ThinLTOGlobalProcessing::getLinkage(const GlobalValue *SGV) {
   // Any local variable that is referenced by an exported function needs
   // to be promoted to global scope. Since we don't currently know which
   // functions reference which local variables/functions, we must treat
   // all as potentially exported if this module is exporting anything.
   if (isModuleExporting()) {
     if (SGV->hasLocalLinkage() && doPromoteLocalToGlobal(SGV))
       return GlobalValue::ExternalLinkage;
     return SGV->getLinkage();
case GlobalValue::WeakODRLinkage:
     else
       return GlobalValue::ExternalLinkage;
 
   case GlobalValue::AppendingLinkage:
     // It would be incorrect to import an appending linkage variable,
     // since it would cause global constructors/destructors to be
     // executed multiple times. This should have already been handled
     // by linkIfNeeded, and we will assert in shouldLinkFromSource
-    // if we try to import, so we simply return AppendingLinkage here
-    // as this helper is called more widely in getLinkedToGlobal.
+    // if we try to import, so we simply return AppendingLinkage.
     return GlobalValue::AppendingLinkage;
 
   case GlobalValue::InternalLinkage:
   case GlobalValue::PrivateLinkage:
     // If we are promoting the local to global scope, it is handled
     // similarly to a normal externally visible global.
     if (doPromoteLocalToGlobal(SGV)) {
       if (doImportAsDefinition(SGV) && !dyn_cast<GlobalAlias>(SGV))
if (!SC)
     return;
   for (GlobalValue *GV2 : ComdatMembers[SC]) {
     if (!GV2->hasLocalLinkage() && shouldInternalizeLinkedSymbols())
       Internalize.insert(GV2->getName());
     Add(*GV2);
   }
 }
 
-void ModuleLinker::processGlobalForThinLTO(GlobalValue &GV) {
+void ThinLTOGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
   if (GV.hasLocalLinkage() &&
       (doPromoteLocalToGlobal(&GV) || isPerformingImport())) {
     GV.setName(getName(&GV));
     GV.setLinkage(getLinkage(&GV));
     if (!GV.hasLocalLinkage())
       GV.setVisibility(GlobalValue::HiddenVisibility);
     if (isModuleExporting())
-      ValuesToLink.insert(&GV);
+      NewExportedValues.insert(&GV);
     return;
   }
   GV.setLinkage(getLinkage(&GV));
 }
 
-void ModuleLinker::processGlobalsForThinLTO() {
-  for (GlobalVariable &GV : SrcM.globals())
+void ThinLTOGlobalProcessing::processGlobalsForThinLTO() {
+  for (GlobalVariable &GV : M.globals())
     processGlobalForThinLTO(GV);
-  for (Function &SF : SrcM)
+  for (Function &SF : M)
     processGlobalForThinLTO(SF);
-  for (GlobalAlias &GA : SrcM.aliases())
+  for (GlobalAlias &GA : M.aliases())
     processGlobalForThinLTO(GA);
 }
 
+bool ThinLTOGlobalProcessing::run() {
+  processGlobalsForThinLTO();
+  return false;
+}
+
 bool ModuleLinker::run() {
   for (const auto &SMEC : SrcM.getComdatSymbolTable()) {
     const Comdat &C = SMEC.getValue();
     if (ComdatsChosen.count(&C))
       continue;
     Comdat::SelectionKind SK;
     bool LinkFromSrc;
     if (getComdatResult(&C, SK, LinkFromSrc))
bool ModuleLinker::run() {
   for (Function &SF : SrcM)
     if (linkIfNeeded(SF))
       return true;
 
   for (GlobalAlias &GA : SrcM.aliases())
     if (linkIfNeeded(GA))
       return true;
 
-  processGlobalsForThinLTO();
+  if (ImportIndex) {
+    ThinLTOGlobalProcessing 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();
     if (!SC)
       continue;
     for (GlobalValue *GV2 : ComdatMembers[SC])
       ValuesToLink.insert(GV2);
bool Linker::linkModules(Module &Dest, std::unique_ptr<Module> Src,
                          unsigned Flags) {
   Linker L(Dest);
   return L.linkInModule(std::move(Src), Flags);
 }
 
 std::unique_ptr<Module>
 llvm::renameModuleForThinLTO(std::unique_ptr<Module> M,
                              const FunctionInfoIndex *Index) {
-  std::unique_ptr<llvm::Module> RenamedModule(
-      new llvm::Module(M->getModuleIdentifier(), M->getContext()));
-  Linker L(*RenamedModule.get());
-  if (L.linkInModule(std::move(M), llvm::Linker::Flags::None, Index))
+  ThinLTOGlobalProcessing ThinLTOProcessing(*M, Index);
+  if (ThinLTOProcessing.run())
     return nullptr;
-  return RenamedModule;
+  return M;
 }
 
 //===----------------------------------------------------------------------===//
 // C API.
 //===----------------------------------------------------------------------===//
 
 static void diagnosticHandler(const DiagnosticInfo &DI, void *C) {
   auto *Message = reinterpret_cast<std::string *>(C);