ThinLTO: Do not take into account whether a definition has multiple copies when promoting.

[pcc]

Summary

We currently do not touch a symbol's linkage in the case where a definition
has a single copy. However, this code is effectively unnecessary: either
the definition is not exported, in which case the internalize phase sets
its linkage to internal, or it is exported, in which case we need to promote
linkage to weak. Those two cases are already handled by existing code.

I believe that the only real functional change here is in the case where we
have a single definition which does not prevail (e.g. because the definition
in a native object file prevails). In that case we now lower linkage to
available_externally following the existing code path for that case.

As a result we can remove the isExported function parameter from the
thinLTOResolveWeakForLinkerInIndex function.

[tejohnson]

This looks like a nice simplification. I'm guessing the special handling for the single copy case was because it predated ThinLTO internalization, but Mehdi can probably say for sure.

Do we have a test case for the single copy non-exported case to ensure that we end up with the same internalized linkage with this change? If not, please add one.

[mehdi_amini]

"Either the definition is not exported, in which case the internalize phase sets its linkage to internal, or it is exported, in which case we need to promote linkage to weak"

We intentionally don't promote linkonce to weak when possible because linkonce -> weak promotion inhibit auto-hidden on MachO.
I should look at it again with the recent local_unnamed_addr attribute to understand exactly where we stand now and how this could affect our codegen.

[pcc]

We intentionally don't promote linkonce to weak when possible because linkonce -> weak promotion inhibit auto-hidden on MachO.
I should look at it again with the recent local_unnamed_addr attribute to understand exactly where we stand now and how this could affect our codegen.

Sorry if I wasn't clear enough, but I believe this to be NFC in the case where the client applies both promotion and internalization to the index. I believe that in every case where I changed linkonce to weak in a test case, the internalize phase would change linkage to internal. We were already upgrading exported symbols from linkonce to weak, see the code I deleted in thinLTOResolveWeakForLinkerGUID. If we do want to apply auto-hidden to these decls, we should probably think about that separately.

(IMHO, the fact that we apply promotion and internalization to the index separately is very confusing, and we should fix that at some point.)

[pcc]

Refresh

[pcc]

Do we have a test case for the single copy non-exported case to ensure that we end up with the same internalized linkage with this change? If not, please add one.

Please see D21915.

In D21883#471613, @pcc wrote:

We intentionally don't promote linkonce to weak when possible because linkonce -> weak promotion inhibit auto-hidden on MachO.
I should look at it again with the recent local_unnamed_addr attribute to understand exactly where we stand now and how this could affect our codegen.

Sorry if I wasn't clear enough, but I believe this to be NFC in the case where the client applies both promotion and internalization to the index. I believe that in every case where I changed linkonce to weak in a test case, the internalize phase would change linkage to internal. We were already upgrading exported symbols from linkonce to weak, see the code I deleted in thinLTOResolveWeakForLinkerGUID. If we do want to apply auto-hidden to these decls, we should probably think about that separately.

(IMHO, the fact that we apply promotion and internalization to the index separately is very confusing, and we should fix that at some point.)

The test cases I added in D21915 should help show that this is NFC.

[tejohnson]

In D21883#471613, @pcc wrote:

(IMHO, the fact that we apply promotion and internalization to the index separately is very confusing, and we should fix that at some point.)

Yep, it is something we have talked about. See also the FIXME in llvm::thinLTOInternalizeModule:

// FIXME: Eventually we should control promotion instead of promoting
// and internalizing again.

[mehdi_amini]

LGTM, thanks.
(sorry for the delay, I'm catching up with two weeks vacations)

llvm/trunk/include/llvm/LTO/LTO.h

 /// via thinLTOResolveWeakForLinkerModule.
 ///
 /// This is done for correctness (if value exported, ensure we always
 /// emit a copy), and compile-time optimization (allow drop of duplicates).
 void thinLTOResolveWeakForLinkerInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
-    function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
         recordNewLinkage);
 
 /// Update the linkages in the given \p Index to mark exported values
 /// as external and non-exported values as internal. The ThinLTO backends
 /// must apply the changes to the Module via thinLTOInternalizeModule.
 void thinLTOInternalizeAndPromoteInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(StringRef, GlobalValue::GUID)> isExported);
 }
 
 #endif

llvm/trunk/lib/LTO/LTO.cpp

std::unique_ptr<Module> loadModuleFromBuffer(const MemoryBufferRef &Buffer,
   return std::move(ModuleOrErr.get());
 }
 
 static void thinLTOResolveWeakForLinkerGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
     DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
-    function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
         recordNewLinkage) {
-  auto HasMultipleCopies = GVSummaryList.size() > 1;
-
   for (auto &S : GVSummaryList) {
     if (GlobalInvolvedWithAlias.count(S.get()))
       continue;
     GlobalValue::LinkageTypes OriginalLinkage = S->linkage();
     if (!GlobalValue::isWeakForLinker(OriginalLinkage))
       continue;
-    // We need to emit only one of these, the first module will keep it,
+    // We need to emit only one of these. The prevailing module will keep it,
     // but turned into a weak, while the others will drop it when possible.
-    if (!HasMultipleCopies) {
-      // Exported Linkonce needs to be promoted to not be discarded.
-      if (GlobalValue::isLinkOnceLinkage(OriginalLinkage) &&
-          isExported(S->modulePath(), GUID))
-        S->setLinkage(GlobalValue::getWeakLinkage(
-            GlobalValue::isLinkOnceODRLinkage(OriginalLinkage)));
-    } else if (isPrevailing(GUID, S.get())) {
+    if (isPrevailing(GUID, S.get())) {
       if (GlobalValue::isLinkOnceLinkage(OriginalLinkage))
         S->setLinkage(GlobalValue::getWeakLinkage(
             GlobalValue::isLinkOnceODRLinkage(OriginalLinkage)));
     }
     // Alias can't be turned into available_externally.
     else if (!isa<AliasSummary>(S.get()) &&
              (GlobalValue::isLinkOnceODRLinkage(OriginalLinkage) ||
               GlobalValue::isWeakODRLinkage(OriginalLinkage)))
       S->setLinkage(GlobalValue::AvailableExternallyLinkage);
     if (S->linkage() != OriginalLinkage)
       recordNewLinkage(S->modulePath(), GUID, S->linkage());
   }
 }
 
 // Resolve Weak and LinkOnce values in the \p Index.
 //
 // We'd like to drop these functions if they are no longer referenced in the
 // current module. However there is a chance that another module is still
 // referencing them because of the import. We make sure we always emit at least
 // one copy.
 void thinLTOResolveWeakForLinkerInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
-    function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
         recordNewLinkage) {
   // We won't optimize the globals that are referenced by an alias for now
   // Ideally we should turn the alias into a global and duplicate the definition
   // when needed.
   DenseSet<GlobalValueSummary *> GlobalInvolvedWithAlias;
   for (auto &I : Index)
     for (auto &S : I.second)
       if (auto AS = dyn_cast<AliasSummary>(S.get()))
         GlobalInvolvedWithAlias.insert(&AS->getAliasee());
 
   for (auto &I : Index)
     thinLTOResolveWeakForLinkerGUID(I.second, I.first, GlobalInvolvedWithAlias,
-                                    isPrevailing, isExported, recordNewLinkage);
+                                    isPrevailing, recordNewLinkage);
 }
 
 static void thinLTOInternalizeAndPromoteGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
     function_ref<bool(StringRef, GlobalValue::GUID)> isExported) {
   for (auto &S : GVSummaryList) {
     if (isExported(S->modulePath(), GUID)) {
       if (GlobalValue::isLocalLinkage(S->linkage()))

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

 
 /// Resolve LinkOnce/Weak symbols. Record resolutions in the \p ResolvedODR map
 /// for caching, and in the \p Index for application during the ThinLTO
 /// backends. This is needed for correctness for exported symbols (ensure
 /// at least one copy kept) and a compile-time optimization (to drop duplicate
 /// copies when possible).
 static void resolveWeakForLinkerInIndex(
     ModuleSummaryIndex &Index,
-    const StringMap<FunctionImporter::ExportSetTy> &ExportLists,
-    const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
     StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>>
         &ResolvedODR) {
 
   DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
   computePrevailingCopies(Index, PrevailingCopy);
 
   auto isPrevailing = [&](GlobalValue::GUID GUID, const GlobalValueSummary *S) {
     const auto &Prevailing = PrevailingCopy.find(GUID);
     // Not in map means that there was only one copy, which must be prevailing.
     if (Prevailing == PrevailingCopy.end())
       return true;
     return Prevailing->second == S;
   };
 
-  auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
-    const auto &ExportList = ExportLists.find(ModuleIdentifier);
-    return (ExportList != ExportLists.end() &&
-            ExportList->second.count(GUID)) ||
-           GUIDPreservedSymbols.count(GUID);
-  };
-
   auto recordNewLinkage = [&](StringRef ModuleIdentifier,
                               GlobalValue::GUID GUID,
                               GlobalValue::LinkageTypes NewLinkage) {
     ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
   };
 
-  thinLTOResolveWeakForLinkerInIndex(Index, isPrevailing, isExported,
-                                     recordNewLinkage);
+  thinLTOResolveWeakForLinkerInIndex(Index, isPrevailing, recordNewLinkage);
 }
 
 // Initialize the TargetMachine builder for a given Triple
 static void initTMBuilder(TargetMachineBuilder &TMBuilder,
                           const Triple &TheTriple) {
   // Set a default CPU for Darwin triples (copied from LTOCodeGenerator).
   // FIXME this looks pretty terrible...
   if (TMBuilder.MCpu.empty() && TheTriple.isOSDarwin()) {
void ThinLTOCodeGenerator::promote(Module &TheModule,
   Index.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists);
 
-  // Convert the preserved symbols set from string to GUID
-  auto GUIDPreservedSymbols =
-  computeGUIDPreservedSymbols(PreservedSymbols, TMBuilder.TheTriple);
-
   // Resolve LinkOnce/Weak symbols.
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
-  resolveWeakForLinkerInIndex(Index, ExportLists, GUIDPreservedSymbols,
-                              ResolvedODR);
+  resolveWeakForLinkerInIndex(Index, ResolvedODR);
 
   thinLTOResolveWeakForLinkerModule(
       TheModule, ModuleToDefinedGVSummaries[ModuleIdentifier]);
 
   promoteModule(TheModule, Index);
 }
 
 /**
void ThinLTOCodeGenerator::run() {
   // We use a std::map here to be able to have a defined ordering when
   // producing a hash for the cache entry.
   // FIXME: we should be able to compute the caching hash for the entry based
   // on the index, and nuke this map.
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
 
   // Resolve LinkOnce/Weak symbols, this has to be computed early because it
   // impacts the caching.
-  resolveWeakForLinkerInIndex(*Index, ExportLists, GUIDPreservedSymbols,
-                              ResolvedODR);
+  resolveWeakForLinkerInIndex(*Index, ResolvedODR);
 
   auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
     const auto &ExportList = ExportLists.find(ModuleIdentifier);
     return (ExportList != ExportLists.end() &&
             ExportList->second.count(GUID)) ||
            GUIDPreservedSymbols.count(GUID);
   };
 

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 = linkonce 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 = linkonce_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 = linkonce 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 = linkonce_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: @linkoncefuncAlias = alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; PROMOTE-DAG: @linkoncefuncWeakAlias = weak alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
-; PROMOTE-DAG: @linkoncefuncLinkonceAlias = linkonce 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 = linkonce_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 = linkonce alias void (...), bitcast (void ()* @weakfunc to void (...)*)
+; PROMOTE-DAG: @weakfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-DAG: @weakfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @weakfunc to void (...)*)
-; PROMOTE-DAG: @weakfuncLinkonceODRAlias = linkonce_odr alias void (...), bitcast (void ()* @weakfunc to void (...)*)
+; PROMOTE-DAG: @weakfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @weakfunc to void (...)*)
 ; PROMOTE-DAG: @weakODRfuncAlias = alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 ; PROMOTE-DAG: @weakODRfuncWeakAlias = weak alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
-; PROMOTE-DAG: @weakODRfuncLinkonceAlias = linkonce alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
+; PROMOTE-DAG: @weakODRfuncLinkonceAlias = weak alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 ; PROMOTE-DAG: @weakODRfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
-; PROMOTE-DAG: @weakODRfuncLinkonceODRAlias = linkonce_odr alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
+; PROMOTE-DAG: @weakODRfuncLinkonceODRAlias = weak_odr alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
 
 ; Only alias to LinkonceODR aliasee can be imported
 ; PROMOTE-DAG: @linkonceODRfuncAlias = alias void (...), bitcast (void ()* @linkonceODRfunc 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 = linkonce alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
+; 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 linkonce_odr void @linkonceODRfunc()
 ; PROMOTE-DAG: define weak_odr void @weakODRfunc()
 ; PROMOTE-DAG: define linkonce void @linkoncefunc()

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

 }
 ; MOD1: define weak void @weakfunc()
 ; MOD2: define weak void @weakfunc()
 define weak void @weakfunc() #0 {
 entry:
   ret void
 }
 
-; MOD1: define linkonce_odr void @linkonceodrfuncInSingleModule()
+; MOD1: define weak_odr void @linkonceodrfuncInSingleModule()
 ; MOD1-INT: define internal void @linkonceodrfuncInSingleModule()
 ; EXPORTED: define weak_odr void @linkonceodrfuncInSingleModule()
 define linkonce_odr void @linkonceodrfuncInSingleModule() #0 {
 entry:
   ret void
 }