[ThinLTO/gold] Perform index-based weak/linkonce resolution

[tejohnson]

Summary

Invoke the weak/linkonce symbol resolution support (already used by
libLTO) that operates via the summary index.

This ensures prevailing linkonce are kept, by making them weak, and
marks preempted copies as available_externally when possible.

With this change, the older support for keeping the prevailing linkonce
(by changing their symbol resolution) is removed.

[tejohnson]

Add back a missing piece that I accidentally removed during patch
cleanup: add save-temps dumping of module just after index-based
optimizations and importing, but before the rest of the optimizations.

[mehdi_amini]

LGTM, with some comments.

tools/gold/gold-plugin.cpp

1023

Is this related to the rest of this patch? How was it handled before?

1364

I'm not sure what is this doing, the comment said "identify symbols referenced by more than a single IR module".

After checking https://github.com/geofft/binutils/blob/master/include/plugin-api.h ; it is Preserving any symbols that is not *exclusively referenced from an IR file* (i.e. the comment above the loop is incomplete: it will "identify symbols that can needs to be preserved outside of LTO, i.e. symbols that LTO can't eliminate under any condition".

1370

GlobalValue::getGUID isn't free, you may want to CSE on these two lines.

[Prazek]

tools/gold/gold-plugin.cpp

1018–1019

I think it would be good to put this comment in an assert.

[tejohnson]

Thanks for the comments, will address as noted below before submitting.

tools/gold/gold-plugin.cpp

1018–1019

Ok, will do when I add this later in a separate patch.

1023

It is related in the sense that the new test uses the .import.bc file to do some checking (before the optimization pipeline kicks in and does inlining, e.g.). But actually, I just changed the test to check the .opt.bc saved temps files and it still works - looks like the linkonce_odr removal doesn't happen until code gen. I will remove this new block (which is also useful for debugging, but not then related to this patch, I can submit separately) and change the test.

1364

Right, I was trying to include both scenarios where we need to preserve this in one comment. I.e. if is is not simply referenced by a single IR modue (referenced from other IR modules or from a non-IR file) then it will not have LDPR_PREVAILING_DEF_IRONLY somewhere and will end up in the Preserve set. I will clarify this in the comment.

1370

Will do.

[mehdi_amini]

tools/gold/gold-plugin.cpp

1023

I'm not sure what you mean with linkonce_odr removal does not happen until codegen?
If you turn it into an available_externally, we should always remove them after the inliner is complete.
If you don't turn them into available_externally but they are inlined, the inline is supposed to remove them:

/// Remove now-dead linkonce functions at the end of
/// processing to avoid breaking the SCC traversal.
bool Inliner::doFinalization(CallGraph &CG) {
  return removeDeadFunctions(CG);
}

[tejohnson]

• Address review comments

test/tools/gold/X86/Inputs/thinlto_weak_resolution.ll

+target triple = "x86_64-unknown-linux-gnu"
+
+; Alias are not optimized
+@linkonceodralias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
+
+; Alias are not optimized
+@linkoncealias = linkonce alias void (), void ()* @linkoncefuncwithalias
+
+; Function with an alias are not optimized
+define linkonce_odr void @linkonceodrfuncwithalias() #0 {
+entry:
+  ret void
+}
+
+; Function with an alias are not optimized
+define linkonce void @linkoncefuncwithalias() #0 {
+entry:
+  ret void
+}
+
+define linkonce_odr void @linkonceodrfunc() #0 {
+entry:
+  ret void
+}
+define linkonce void @linkoncefunc() #0 {
+entry:
+  ret void
+}
+define weak_odr void @weakodrfunc() #0 {
+entry:
+  ret void
+}
+define weak void @weakfunc() #0 {
+entry:
+  ret void
+}
+

test/tools/gold/X86/thinlto_weak_resolution.ll

+; RUN: opt -module-summary %s -o %t.o
+; RUN: opt -module-summary %p/Inputs/thinlto_weak_resolution.ll -o %t2.o
+
+; Verify that prevailing weak for linker symbol is kept.
+; Note that gold picks the first copy of a function as the prevailing one,
+; so listing %t.o first is sufficient to ensure that its copies are prevailing.
+; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:     --plugin-opt=thinlto \
+; RUN:     --plugin-opt=save-temps \
+; RUN:     -shared \
+; RUN:     -o %t3.o %t.o %t2.o
+
+; RUN: llvm-nm %t3.o | FileCheck %s
+; CHECK: weakfunc
+
+; All of the preempted functions should have been eliminated (the plugin will
+; not link them in).
+; RUN: llvm-dis %t2.o.import.bc -o - | FileCheck --check-prefix=OPT2 %s
+; OPT2-NOT: @
+
+; RUN: llvm-dis %t.o.import.bc -o - | FileCheck --check-prefix=OPT %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+
+define i32 @main() #0 {
+entry:
+  call void @linkonceodralias()
+  call void @linkoncealias()
+  call void @linkonceodrfuncwithalias()
+  call void @linkoncefuncwithalias()
+  call void @linkonceodrfunc()
+  call void @linkoncefunc()
+  call void @weakodrfunc()
+  call void @weakfunc()
+  call void @linkonceodrfuncInSingleModule()
+  ret i32 0
+}
+
+; Alias are resolved
+; OPT: @linkonceodralias = weak_odr alias void (), void ()* @linkonceodrfuncwithalias
+@linkonceodralias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
+
+; Alias are resolved
+; OPT: @linkoncealias = weak alias void (), void ()* @linkoncefuncwithalias
+@linkoncealias = linkonce alias void (), void ()* @linkoncefuncwithalias
+
+; Function with an alias are not optimized
+; OPT: define linkonce_odr void @linkonceodrfuncwithalias()
+define linkonce_odr void @linkonceodrfuncwithalias() #0 {
+entry:
+  ret void
+}
+
+; Function with an alias are not optimized
+; OPT: define linkonce void @linkoncefuncwithalias()
+define linkonce void @linkoncefuncwithalias() #0 {
+entry:
+  ret void
+}
+
+; OPT: define weak_odr void @linkonceodrfunc()
+define linkonce_odr void @linkonceodrfunc() #0 {
+entry:
+  ret void
+}
+; OPT: define weak void @linkoncefunc()
+define linkonce void @linkoncefunc() #0 {
+entry:
+  ret void
+}
+; OPT: define weak_odr void @weakodrfunc()
+define weak_odr void @weakodrfunc() #0 {
+entry:
+  ret void
+}
+; OPT: define weak void @weakfunc()
+define weak void @weakfunc() #0 {
+entry:
+  ret void
+}
+
+; OPT: weak_odr void @linkonceodrfuncInSingleModule()
+define linkonce_odr void @linkonceodrfuncInSingleModule() #0 {
+entry:
+  ret void
+}

tools/gold/gold-plugin.cpp

if (!GV) {
       freeSymName(Sym);
       continue; // Asm symbol.
     }
 
     ResolutionInfo &Res = ResInfo[Sym.name];
     if (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP && !Res.IsLinkonceOdr)
       Resolution = LDPR_PREVAILING_DEF;
 
-    // In ThinLTO mode change all prevailing resolutions to LDPR_PREVAILING_DEF.
-    // For ThinLTO the IR files are compiled through the backend independently,
-    // so we need to ensure that any prevailing linkonce copy will be emitted
-    // into the object file by making it weak. Additionally, we can skip the
-    // IRONLY handling for internalization, which isn't performed in ThinLTO
-    // mode currently anyway.
-    if (options::thinlto && (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP ||
-                             Resolution == LDPR_PREVAILING_DEF_IRONLY))
-      Resolution = LDPR_PREVAILING_DEF;
-
     GV->setUnnamedAddr(Res.UnnamedAddr);
     GV->setVisibility(Res.Visibility);
 
     // Override gold's resolution for common symbols. We want the largest
     // one to win.
     if (GV->hasCommonLinkage()) {
       if (Resolution == LDPR_PREVAILING_DEF_IRONLY)
         Res.CommonInternal = true;
return std::unique_ptr<TargetMachine>(TheTarget->createTargetMachine(
       TripleStr, options::mcpu, FeaturesString, Options, RelocationModel,
       CodeModel::Default, CGOptLevel));
 }
 
 void CodeGen::runLTOPasses() {
   M->setDataLayout(TM->createDataLayout());
 
   if (CombinedIndex) {
+    // Apply summary-based LinkOnce/Weak resolution decisions.
+    thinLTOResolveWeakForLinkerModule(*M, *DefinedGlobals);
+
     // Apply summary-based internalization decisions. Skip if there are no
     // defined globals from the summary since not only is it unnecessary, but
     // if this module did not have a summary section the internalizer will
     // assert if it finds any definitions in this module that aren't in the
     // DefinedGlobals set.
     if (!DefinedGlobals->empty())
       thinLTOInternalizeModule(*M, *DefinedGlobals);
 
     // Create a loader that will parse the bitcode from the buffers
     // in the ModuleMap.
     ModuleLoader Loader(M->getContext(), *ModuleMap);
 
     // Perform function importing.
     FunctionImporter Importer(*CombinedIndex, Loader);
     Importer.importFunctions(*M, *ImportList);
+
+    // If save-temps is specified, save the module just after importing
+    // and index-based optimizations, but before the rest of the optimizations.
+    if (options::TheOutputType == options::OT_SAVE_TEMPS) {
+      // Always expect a provided filename in the ThinLTO backend.
+      assert(!SaveTempsFilename.empty());
+      SmallString<128> OptFilename(SaveTempsFilename);
+      OptFilename += ".import.bc";
+      saveBCFile(OptFilename, *M);
+    }
   }
 
   legacy::PassManager passes;
   passes.add(createTargetTransformInfoWrapperPass(TM->getTargetIRAnalysis()));
 
   PassManagerBuilder PMB;
   PMB.LibraryInfo = new TargetLibraryInfoImpl(Triple(TM->getTargetTriple()));
   PMB.Inliner = createFunctionInliningPass();
static ld_plugin_status thinLTOLink(raw_fd_ostream *ApiFile) {
   // Map to own RAII objects that manage the file opening and releasing
   // interfaces with gold.
   DenseMap<void *, std::unique_ptr<PluginInputFile>> HandleToInputFile;
 
   // Keep track of symbols that must not be internalized because they
   // are referenced outside of a single IR module.
   DenseSet<GlobalValue::GUID> Preserve;
 
+  // Keep track of the prevailing copy for each GUID, for use in resolving
+  // weak linkages.
+  DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
+
   ModuleSummaryIndex CombinedIndex;
   uint64_t NextModuleId = 0;
   for (claimed_file &F : Modules) {
     if (!HandleToInputFile.count(F.leader_handle))
       HandleToInputFile.insert(std::make_pair(
           F.leader_handle, llvm::make_unique<PluginInputFile>(F.handle)));
     // Pass this into getModuleSummaryIndexForFile
     const void *View = getSymbolsAndView(F);
     if (!View)
       continue;
 
     MemoryBufferRef ModuleBuffer(StringRef((const char *)View, F.filesize),
                                  F.name);
     assert(ModuleMap.find(ModuleBuffer.getBufferIdentifier()) ==
                ModuleMap.end() &&
            "Expect unique Buffer Identifier");
     ModuleMap[ModuleBuffer.getBufferIdentifier()] = ModuleBuffer;
 
     std::unique_ptr<ModuleSummaryIndex> Index = getModuleSummaryIndexForFile(F);
 
-    // Skip files without a module summary.
-    if (Index)
-      CombinedIndex.mergeFrom(std::move(Index), ++NextModuleId);
-
     // Use gold's symbol resolution information to identify symbols referenced
     // by more than a single IR module (before importing, which is checked
-    // separately).
+    // separately). Also track the prevailing copy for later symbol resolution.
     for (auto &Sym : F.syms) {
       ld_plugin_symbol_resolution Resolution =
           (ld_plugin_symbol_resolution)Sym.resolution;
       if (Resolution != LDPR_PREVAILING_DEF_IRONLY)
         Preserve.insert(GlobalValue::getGUID(Sym.name));
+
+      if (Index && (Resolution == LDPR_PREVAILING_DEF ||
+                    Resolution == LDPR_PREVAILING_DEF_IRONLY ||
+                    Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP))
+        PrevailingCopy[GlobalValue::getGUID(Sym.name)] =
+            Index->getGlobalValueSummary(GlobalValue::getGUID(Sym.name));
     }
+
+    // Skip files without a module summary.
+    if (Index)
+      CombinedIndex.mergeFrom(std::move(Index), ++NextModuleId);
   }
 
   // Collect for each module the list of function it defines (GUID ->
   // Summary).
   StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>>
       ModuleToDefinedGVSummaries(NextModuleId);
   CombinedIndex.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   StringMap<FunctionImporter::ImportMapTy> ImportLists(NextModuleId);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(NextModuleId);
   ComputeCrossModuleImport(CombinedIndex, ModuleToDefinedGVSummaries,
                            ImportLists, ExportLists);
 
+  auto isPrevailing = [&](GlobalValue::GUID GUID, const GlobalValueSummary *S) {
+    const auto &Prevailing = PrevailingCopy.find(GUID);
+    assert(Prevailing != PrevailingCopy.end());
+    return Prevailing->second == S;
+  };
+
   // Callback for internalization, to prevent internalization of symbols
   // that were not candidates initially, and those that are being imported
   // (which introduces new cross references).
   auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
     const auto &ExportList = ExportLists.find(ModuleIdentifier);
     return (ExportList != ExportLists.end() &&
             ExportList->second.count(GUID)) ||
            Preserve.count(GUID);
   };
 
+  thinLTOResolveWeakForLinkerInIndex(
+      CombinedIndex, isPrevailing,
+      [](StringRef ModuleIdentifier, GlobalValue::GUID GUID,
+         GlobalValue::LinkageTypes NewLinkage) {});
+
   // Use global summary-based analysis to identify symbols that can be
   // internalized (because they aren't exported or preserved as per callback).
   // Changes are made in the index, consumed in the ThinLTO backends.
   thinLTOInternalizeAndPromoteInIndex(CombinedIndex, isExported);
 
   if (options::thinlto_emit_imports_files && !options::thinlto_index_only)
     message(LDPL_WARNING,
             "thinlto-emit-imports-files ignored unless thinlto-index-only");