[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 ↗

(On Diff #63813)

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

1364 ↗

(On Diff #63813)

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 ↗

(On Diff #63813)

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

[Prazek]

tools/gold/gold-plugin.cpp

1018–1019 ↗

(On Diff #63813)

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 ↗

(On Diff #63813)

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

1023 ↗

(On Diff #63813)

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 ↗

(On Diff #63813)

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 ↗

(On Diff #63813)

Will do.

[mehdi_amini]

tools/gold/gold-plugin.cpp

1023 ↗

(On Diff #63813)

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

llvm/trunk/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
+}
+

llvm/trunk/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.opt.bc -o - | FileCheck --check-prefix=OPT2 %s
+; OPT2-NOT: @
+
+; RUN: llvm-dis %t.o.opt.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
+}

llvm/trunk/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);
 
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).
+    // by more than a single IR module (i.e. referenced by multiple IR modules
+    // or by a non-IR module). Cross references introduced by importing are
+    // checked separately via the export lists. 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;
+      GlobalValue::GUID SymGUID = GlobalValue::getGUID(Sym.name);
       if (Resolution != LDPR_PREVAILING_DEF_IRONLY)
-        Preserve.insert(GlobalValue::getGUID(Sym.name));
+        Preserve.insert(SymGUID);
+
+      if (Index && (Resolution == LDPR_PREVAILING_DEF ||
+                    Resolution == LDPR_PREVAILING_DEF_IRONLY ||
+                    Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP))
+        PrevailingCopy[SymGUID] = Index->getGlobalValueSummary(SymGUID);
     }
+
+    // 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");