[ThinLTO] Only promote exported locals as marked in index

[tejohnson]

Summary

We have always speculatively promoted all renamable local values
(except const non-address taken variables) for both the exporting
and importing module. We would then internalize them back based on
the ThinLink results if they weren't actually exported. This is
inefficient, and results in unnecessary renames. It also meant we
had to check the non-renamability of a value in the summary, which
was already checked during function importing analysis in the ThinLink.

Made renameModuleForThinLTO (which does the promotion/renaming) instead
use the index when exporting, to avoid unnecessary renames/promotions.
For importing modules, we can simply promoted all values as any local
we import by definition is exported and needs promotion.

This required changes to the method used by the FunctionImport pass
(only invoked from 'opt' for testing) and when invoked from llvm-link,
since neither does a ThinLink. We simply conservatively mark all locals
in the index as promoted, which preserves the current aggressive
promotion behavior.

I also needed to change an llvm-lto based test where we had previously
been aggressively promoting values that weren't importable (aliasees),
but now will not promote.

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

553 ↗

(On Diff #77385)

Not clear why you init TMBuilder, while you only seem to use the Triple?

561 ↗

(On Diff #77385)

Is it clang-formatted?

lib/Transforms/Utils/FunctionImportUtils.cpp

80 ↗

(On Diff #77385)

why can't we just promote what is needed?

[tejohnson]

lib/LTO/ThinLTOCodeGenerator.cpp

553 ↗

(On Diff #77385)

Hmm, I cloned this from ThinLTOCodeGenerator::internalize, perhaps it isn't needed in either place. I removed the init from here for now.

561 ↗

(On Diff #77385)

Yes. I modified the spacing on this line, re-ran clang-format, and got back the same thing.

lib/Transforms/Utils/FunctionImportUtils.cpp

80 ↗

(On Diff #77385)

See the comment just above this - when we are importing we may not have a summary in the distributed backend case (we only get the summaries for things we want to import as defs, not all references in the module). At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the resulting module.

[tejohnson]

Address comments

[mehdi_amini]

LGTM.

lib/Transforms/Utils/FunctionImportUtils.cpp

80 ↗

(On Diff #77385)

See the comment just above this - when we are importing we may not have a summary in the distributed backend case (we only get the summaries for things we want to import as defs, not all references in the module).

That's annoying.

At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the resulting module.

Sure, but I'm still not fan: it means we can't catch error at this stage.

[tejohnson]

In D26467#594481, @mehdi_amini wrote:

LGTM.

[tejohnson]

(Hit send too early on previous response)

lib/Transforms/Utils/FunctionImportUtils.cpp

80 ↗

(On Diff #77385)

At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the >> resulting module.

Sure, but I'm still not fan: it means we can't catch error at this stage.

Agree, it would be better to be able to handle these based on the index. However, this means including summaries of references (not to be imported as defs) assuming that the backend's invocation of llvm::ComputeCrossModuleImportForModule will always make the same decisions. If someone for example accidentally passes tuning options like -import-instr-limit to the backend and not the thin link this could break things if a superset of things originally decided for import by the thin link are then imported. A more robust solution would be to add another flag to the (combined) index that identifies summaries to import as definitions.

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

 /**
  * Perform promotion and renaming of exported internal functions.
  * Index is updated to reflect linkage changes from weak resolution.
  */
 void ThinLTOCodeGenerator::promote(Module &TheModule,
                                    ModuleSummaryIndex &Index) {
   auto ModuleCount = Index.modulePaths().size();
   auto ModuleIdentifier = TheModule.getModuleIdentifier();
+
   // Collect for each module the list of function it defines (GUID -> Summary).
   StringMap<GVSummaryMapTy> ModuleToDefinedGVSummaries;
   Index.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists);
 
   // Resolve LinkOnce/Weak symbols.
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
   resolveWeakForLinkerInIndex(Index, ResolvedODR);
 
   thinLTOResolveWeakForLinkerModule(
       TheModule, ModuleToDefinedGVSummaries[ModuleIdentifier]);
 
+  // Convert the preserved symbols set from string to GUID
+  auto GUIDPreservedSymbols = computeGUIDPreservedSymbols(
+      PreservedSymbols, Triple(TheModule.getTargetTriple()));
+
+  // Promote the exported values in the index, so that they are promoted
+  // in the module.
+  auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
+    const auto &ExportList = ExportLists.find(ModuleIdentifier);
+    return (ExportList != ExportLists.end() &&
+            ExportList->second.count(GUID)) ||
+           GUIDPreservedSymbols.count(GUID);
+  };
+  thinLTOInternalizeAndPromoteInIndex(Index, isExported);
+
   promoteModule(TheModule, Index);
 }
 
 /**
  * Perform cross-module importing for the module identified by ModuleIdentifier.
  */
 void ThinLTOCodeGenerator::crossModuleImport(Module &TheModule,
                                              ModuleSummaryIndex &Index) {

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

if (!SummaryFile.empty()) {
     Index = IndexPtr.get();
   }
 
   // First step is collecting the import list.
   FunctionImporter::ImportMapTy ImportList;
   ComputeCrossModuleImportForModule(M.getModuleIdentifier(), *Index,
                                     ImportList);
 
+  // Conservatively mark all internal values as promoted. This interface is
+  // only used when doing importing via the function importing pass. The pass
+  // is only enabled when testing importing via the 'opt' tool, which does
+  // not do the ThinLink that would normally determine what values to promote.
+  for (auto &I : *Index) {
+    for (auto &S : I.second) {
+      if (GlobalValue::isLocalLinkage(S->linkage()))
+        S->setLinkage(GlobalValue::ExternalLinkage);
+    }
+  }
+
   // Next we need to promote to global scope and rename any local values that
   // are potentially exported to other modules.
   if (renameModuleForThinLTO(M, *Index, nullptr)) {
     errs() << "Error renaming module\n";
     return false;
   }
 
   // Perform the import now.

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

bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(
   if (GVar && GVar->isConstant() && GVar->hasGlobalUnnamedAddr())
     return false;
 
   // If we are exporting, we need to see whether this value is marked
   // as NoRename in the summary. If we are importing, we may not have
   // a summary in the distributed backend case (only summaries for values
   // importes as defs, not references, are included in the index passed
   // to the distributed backends).
+  if (isPerformingImport()) {
+    // 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;
+  }
+
+  // When exporting, consult the index.
   auto Summaries = ImportIndex.findGlobalValueSummaryList(SGV->getGUID());
-  if (Summaries == ImportIndex.end())
-    // Assert that this is an import - we should always have access to the
-    // summary when exporting.
-    assert(isPerformingImport() &&
-           "Missing summary for global value when exporting");
-  else {
-    assert(Summaries->second.size() == 1 && "Local has more than one summary");
-    if (Summaries->second.front()->noRename()) {
-      assert((isModuleExporting() || !GlobalsToImport->count(SGV)) &&
-             "Imported a non-renamable local value");
-      return false;
-    }
+  assert(Summaries != ImportIndex.end() &&
+  auto Linkage = Summaries->second.front()->linkage();
+  if (!GlobalValue::isLocalLinkage(Linkage)) {
+    assert(!Summaries->second.front()->noRename());
+    return true;
   }
 
-  // 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
-  // summary index).
-  return true;
+  return false;
 }
 
 std::string FunctionImportGlobalProcessing::getName(const GlobalValue *SGV,
                                                     bool DoPromote) {
   // 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

llvm/trunk/test/ThinLTO/X86/alias_import.ll

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %p/Inputs/alias_import.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc %t2.bc
 ; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t.index.bc %t2.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=PROMOTE
 ; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t.index.bc %t2.bc -o - | llvm-lto -thinlto-action=internalize -thinlto-index %t.index.bc -thinlto-module-id=%t2.bc - -o - | llvm-dis -o - | FileCheck %s --check-prefix=PROMOTE-INTERNALIZE
 ; RUN: llvm-lto -thinlto-action=import -thinlto-index %t.index.bc %t1.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT
 
 ;
 ; Alias can't point to "available_externally", so we can only import an alias
 ; when we can import the aliasee with a linkage that won't be
 ; available_externally, i.e linkOnceODR. (FIXME this limitation could be lifted)
 ; PROMOTE-DAG: @globalfuncAlias = alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-DAG: @globalfuncWeakAlias = weak alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-DAG: @globalfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-DAG: @globalfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-DAG: @globalfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @globalfunc to void (...)*)
-; PROMOTE-DAG: @internalfuncAlias = alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-DAG: @internalfuncWeakAlias = weak alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-DAG: @internalfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-DAG: @internalfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-DAG: @internalfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
+; PROMOTE-DAG: @internalfuncAlias = alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-DAG: @internalfuncWeakAlias = weak alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-DAG: @internalfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-DAG: @internalfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-DAG: @internalfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @internalfunc to void (...)*)
 ; PROMOTE-DAG: @linkoncefuncAlias = alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-DAG: @linkoncefuncWeakAlias = weak alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-DAG: @linkoncefuncLinkonceAlias = weak alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-DAG: @linkoncefuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-DAG: @linkoncefuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-DAG: @weakfuncAlias = alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-DAG: @weakfuncWeakAlias = weak alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-DAG: @weakfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @weakfunc to void (...)*)

 ; PROMOTE-DAG: @linkonceODRfuncWeakAlias = weak alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-DAG: @linkonceODRfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; Amongst these that are imported, check that we promote only linkonce->weak
 ; PROMOTE-DAG: @linkonceODRfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-DAG: @linkonceODRfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 
 ; These will be imported, check the linkage/renaming after promotion
 ; PROMOTE-DAG: define void @globalfunc()
-; PROMOTE-DAG: define hidden void @internalfunc.llvm.0()
+; PROMOTE-DAG: define internal void @internalfunc()
 ; PROMOTE-DAG: define weak_odr void @linkonceODRfunc()
 ; PROMOTE-DAG: define weak_odr void @weakODRfunc()
 ; PROMOTE-DAG: define weak void @linkoncefunc()
 ; PROMOTE-DAG: define weak void @weakfunc()
 
 ; On the import side now, verify that aliases to a linkonce_odr are imported, but the weak/linkonce (we can't inline them)
 ; IMPORT-DAG:  declare void @linkonceODRfuncWeakAlias
 ; IMPORT-DAG: declare void @linkonceODRfuncLinkonceAlias

 ; Promotion + internalization should internalize all of these, except for aliases of
 ; linkonce_odr functions, because alias to linkonce_odr are the only aliases we will
 ; import (see selectCallee() in FunctionImport.cpp).
 ; PROMOTE-INTERNALIZE-DAG: @globalfuncAlias = internal alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @globalfuncWeakAlias = internal alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @globalfuncLinkonceAlias = internal alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @globalfuncWeakODRAlias = internal alias void (...), bitcast (void ()* @globalfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @globalfuncLinkonceODRAlias = internal alias void (...), bitcast (void ()* @globalfunc to void (...)*)
-; PROMOTE-INTERNALIZE-DAG: @internalfuncAlias = internal alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-INTERNALIZE-DAG: @internalfuncWeakAlias = internal alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-INTERNALIZE-DAG: @internalfuncLinkonceAlias = internal alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-INTERNALIZE-DAG: @internalfuncWeakODRAlias = internal alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
-; PROMOTE-INTERNALIZE-DAG: @internalfuncLinkonceODRAlias = internal alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
+; PROMOTE-INTERNALIZE-DAG: @internalfuncAlias = internal alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-INTERNALIZE-DAG: @internalfuncWeakAlias = internal alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-INTERNALIZE-DAG: @internalfuncLinkonceAlias = internal alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-INTERNALIZE-DAG: @internalfuncWeakODRAlias = internal alias void (...), bitcast (void ()* @internalfunc to void (...)*)
+; PROMOTE-INTERNALIZE-DAG: @internalfuncLinkonceODRAlias = internal alias void (...), bitcast (void ()* @internalfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncAlias = alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncWeakAlias = internal alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncLinkonceAlias = internal alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakODRfuncAlias = internal alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakODRfuncWeakAlias = internal alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakODRfuncLinkonceAlias = internal alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakODRfuncWeakODRAlias = internal alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakODRfuncLinkonceODRAlias = internal alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkoncefuncAlias = internal alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkoncefuncWeakAlias = internal alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkoncefuncLinkonceAlias = internal alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkoncefuncWeakODRAlias = internal alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @linkoncefuncLinkonceODRAlias = internal alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakfuncAlias = internal alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakfuncWeakAlias = internal alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakfuncLinkonceAlias = internal alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakfuncWeakODRAlias = internal alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: @weakfuncLinkonceODRAlias = internal alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-INTERNALIZE-DAG: define internal void @globalfunc()
-; PROMOTE-INTERNALIZE-DAG: define internal void @internalfunc.llvm.0()
+; PROMOTE-INTERNALIZE-DAG: define internal void @internalfunc()
 ; PROMOTE-INTERNALIZE-DAG: define internal void @linkonceODRfunc()
 ; PROMOTE-INTERNALIZE-DAG: define internal void @weakODRfunc()
 ; PROMOTE-INTERNALIZE-DAG: define internal void @linkoncefunc()
 ; PROMOTE-INTERNALIZE-DAG: define internal void @weakfunc()
 
 define i32 @main() #0 {
 entry:
   call void @globalfuncAlias()

llvm/trunk/tools/llvm-link/llvm-link.cpp

for (const auto &File : Files) {
     }
 
     // If a module summary index is supplied, load it so linkInModule can treat
     // local functions/variables as exported and promote if necessary.
     if (!SummaryIndex.empty()) {
       std::unique_ptr<ModuleSummaryIndex> Index =
           ExitOnErr(llvm::getModuleSummaryIndexForFile(SummaryIndex));
 
+      // Conservatively mark all internal values as promoted, since this tool
+      // does not do the ThinLink that would normally determine what values to
+      // promote.
+      for (auto &I : *Index) {
+        for (auto &S : I.second) {
+          if (GlobalValue::isLocalLinkage(S->linkage()))
+            S->setLinkage(GlobalValue::ExternalLinkage);
+        }
+      }
+
       // Promotion
       if (renameModuleForThinLTO(*M, *Index))
         return true;
     }
 
     if (Verbose)
       errs() << "Linking in '" << File << "'\n";