[ThinLTO] Enable AutoHide on symbols with local_unnamed_addr

[steven_wu]

Summary

Teach thinLTO to analyze and auto hide local_unnamed_addr symbols if
possible.

[pcc]

The last time this was attempted (D28978) we established that the linker can do this itself. I don't think the situation has changed since then.

[steven_wu]

In D43361#1009486, @pcc wrote:

The last time this was attempted (D28978) we established that the linker can do this itself. I don't think the situation has changed since then.

I didn't know that we have discussed about this once. This issue comes up again because when thinLTO can promote linkonce_odr to weak_odr (in order to keep the symbol, not to disable auto hide), thus it lost auto hide before the object reaches the linker.

There is an alternative which is to teach linker to deduct auto hide property from the bitcode file and overwrite the linkage type afterwards. For C API, there is a new scope LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN can be used to do that but that will be forced the work onto all the linker which is probably not desirable but it solves the same problem.

[pcc]

There is an alternative which is to teach linker to deduct auto hide property from the bitcode file and overwrite the linkage type afterwards. For C API, there is a new scope LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN can be used to do that but that will be forced the work onto all the linker which is probably not desirable but it solves the same problem.

This seems better to me. Ideally the linker should be reading and resolving symbols from bitcode files in the same way as regular object files, including the auto-hide bit. Linkers would need something similar for any other properties that are contained only in bitcode files but not in object files (such as a general address-taken bit).

[steven_wu]

In D43361#1009616, @pcc wrote:

There is an alternative which is to teach linker to deduct auto hide property from the bitcode file and overwrite the linkage type afterwards. For C API, there is a new scope LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN can be used to do that but that will be forced the work onto all the linker which is probably not desirable but it solves the same problem.

This seems better to me. Ideally the linker should be reading and resolving symbols from bitcode files in the same way as regular object files, including the auto-hide bit. Linkers would need something similar for any other properties that are contained only in bitcode files but not in object files (such as a general address-taken bit).

But this is extremely fragile, especially through C API. Linker can never trust 100% of all the information provided in the bitcode because there can be optimizations in the LTO pipeline that modify/add/delete any symbols or attributes. Unless the linker knows about all the optimizations, it is risky for linker to redo the linkage change from the optimizers and expect that there are no side affects. In this case, for example, if someone implement an optimization that can introduce code that taken the address of local_unnamed_addr and remove the attributes, then linker is not allowed to auto hide, even through all the original bitcode says unnamed_addr.

[pcc]

In D43361#1010401, @steven_wu wrote:

In D43361#1009616, @pcc wrote:

There is an alternative which is to teach linker to deduct auto hide property from the bitcode file and overwrite the linkage type afterwards. For C API, there is a new scope LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN can be used to do that but that will be forced the work onto all the linker which is probably not desirable but it solves the same problem.

This seems better to me. Ideally the linker should be reading and resolving symbols from bitcode files in the same way as regular object files, including the auto-hide bit. Linkers would need something similar for any other properties that are contained only in bitcode files but not in object files (such as a general address-taken bit).

But this is extremely fragile, especially through C API. Linker can never trust 100% of all the information provided in the bitcode because there can be optimizations in the LTO pipeline that modify/add/delete any symbols or attributes. Unless the linker knows about all the optimizations, it is risky for linker to redo the linkage change from the optimizers and expect that there are no side affects. In this case, for example, if someone implement an optimization that can introduce code that taken the address of local_unnamed_addr and remove the attributes, then linker is not allowed to auto hide, even through all the original bitcode says unnamed_addr.

The unnamed_addr attribute means that the address is semantically significant. If a pass changes whether a global's address is semantically significant, that is a change in semantics and a bug in the pass.

[pcc]

The unnamed_addr attribute means that the address is semantically significant

I mean, that the address is *not* semantically significant.

[steven_wu]

I am using local_unnamed_addr as an example. Maybe you are right but I think my point is still valid. That is, LTO optimization and code generation can modify the symbol table from the bitcode file. It is already the case that symbols can be internalized, removed, backend can also insert new symbols. Unless there is a rule saying LTO passes cannot modify the linkage or visibility or any other attributes to the symbols, linker cannot trust whatever the bitcode symbol table says and firmly it will not be changed for some good reason.

In this case, if the unnamed_addr attributes are not illegal to change, the linkage can be changed which effectively disable autohide on the symbol.

[• espindola]

Can this be closed?

[• espindola]

If LTO does change the IR in some way that would drop local_unnamed_addr, it will not be present in the .o that is passed to the linker.

So the linker can decide if it is valid to hide the symbol by looking at all the input files, bitcode or not, including the ones produced by LTO.

[steven_wu]

So the original of this patch is thinLTO is promoting linkonce_odr to weak_odr. Think the following two situations:

1. linkonce_odr + local_unnamed_addr --> linkonce_odr

compiler is dropping unnamed_addr, it is not safe to hide.

2. linkonce_odr + local_unnamed_addr --> weak_odr + local_unnamed_addr

this is the linkage promotion by thinLTO, it is still 'safe' to hide.

For the linker point of view (at least ld64), both of them started with "weak auto-hide" in IR and turns into "weak" in object file. The only thing like can do safely is export the weak symbol.

[• espindola]

In D43361#1039615, @steven_wu wrote:

So the original of this patch is thinLTO is promoting linkonce_odr to weak_odr. Think the following two situations:

1. linkonce_odr + local_unnamed_addr --> linkonce_odr

compiler is dropping unnamed_addr, it is not safe to hide.

2. linkonce_odr + local_unnamed_addr --> weak_odr + local_unnamed_addr

this is the linkage promotion by thinLTO, it is still 'safe' to hide.

The solution is to use canBeOmittedFromSymbolTable from include/llvm/Object/IRSymtab.h.

The symbol table is computed earlier and so the above predicate still returns 1. Your examples work with lld and ThinLTO if you want to check the implementation details.

[steven_wu]

The solution is to use canBeOmittedFromSymbolTable from include/llvm/Object/IRSymtab.h.

The symbol table is computed earlier and so the above predicate still returns 1. Your examples work with lld and ThinLTO if you want to check the implementation details.

It is easy to get 1 correct. This patch is to get 2 to be hidden using LTO.

Sure, linker can consult the symbol table in IRSymtab but the problem is how much can it be trusted? LTO can already add/remove symbols from IRSymtab. Is changing symbol linkage/visibility legal during LTO?

[• espindola]

In D43361#1039665, @steven_wu wrote:

The solution is to use canBeOmittedFromSymbolTable from include/llvm/Object/IRSymtab.h.

The symbol table is computed earlier and so the above predicate still returns 1. Your examples work with lld and ThinLTO if you want to check the implementation details.

It is easy to get 1 correct. This patch is to get 2 to be hidden using LTO.

Using the IRSymtab also solves 2.

Sure, linker can consult the symbol table in IRSymtab but the problem is how much can it be trusted? LTO can already add/remove symbols from IRSymtab. Is changing symbol linkage/visibility legal during LTO?

It can be trusted. That is what lld and the gold plugin are using. The only theoretical change I can think of is a pass adding a use and dropping local_unnamed_addr, but the linker will see that in the .o file that LTO produces.

[steven_wu]

I might have missed something. Can you be more specific about how it is going to work?

Let's say IRSymtab has symbol "_foo" and it is weak and it can be omitted from symbol table. After LTO codegen, linker sees that "_foo" is weak in the object file without "auto hide" attribute. How can linker figure out if it should hide the symbol or not?

Actually, how does ELF/COFF linker know that if LTO decides to drop local_unnamed_addr (since only macho has flags to indicate that)? Is lld/gold looking at the object file to figure out if there are uses for the addr for this symbol?

[• espindola]

Actually, how does ELF/COFF linker know that if LTO decides to drop local_unnamed_addr (since only macho has flags to indicate that)? Is lld/gold looking at the object file to figure out if there are uses for the addr for this symbol?

Right now the symbol is only dropped if it is used only from IR files. Any use from ELF counts as cannot be hidden. This is indeed the simple case as the symbol can be internalized.

But it does point out that we have an implicit assumption: no pass drops local_unnamed_addr.

If we had a strange_pass that did that, what we would get is

• Linker gets true for every call to canBeOmittedFromSymbolTable.
• Linker sets VisibleToRegularObj to false for every file.
• LLVM internalizes the GV
• strange_pass adds a use of the address of GV
• we now have an address dependency that can fail.

I am tempered to just document in the language reference that it is invalid for a pass to drop local_unnamed_addr. With that the linker job is even simpler, it doesn't need to check the LTO produced files, just canBeOmittedFromSymbolTable in very IR symbol table.

Peter, what do you think?

[pcc]

I am tempered to just document in the language reference that it is invalid for a pass to drop local_unnamed_addr. With that the linker job is even simpler, it doesn't need to check the LTO produced files, just canBeOmittedFromSymbolTable in very IR symbol table.

Peter, what do you think?

I agree. If a pass drops local_unnamed_addr from a global, it means that the pass has changed the program semantics such that the global's address is significant. But passes are not allowed to change semantics, therefore dropping local_unnamed_addr is invalid.

[steven_wu]

If we all agree that dropping local_unnamed_addr is invalid, I can abandon this. Thanks for the feedback!

include/llvm/IR/ModuleSummaryIndex.h

void setWithGlobalValueDeadStripping() {
     WithGlobalValueDeadStripping = true;
   }
 
   bool isGlobalValueLive(const GlobalValueSummary *GVS) const {
     return !WithGlobalValueDeadStripping || GVS->isLive();
   }
   bool isGUIDLive(GlobalValue::GUID GUID) const;
 
+  bool canGlobalValueAutoHide(const GlobalValue *GV) const {
+    return canGUIDAutoHide(GV->getGUID());
+  }
+  bool canGUIDAutoHide(GlobalValue::GUID GUID) const;
+
   /// Return a ValueInfo for GUID if it exists, otherwise return ValueInfo().
   ValueInfo getValueInfo(GlobalValue::GUID GUID) const {
     auto I = GlobalValueMap.find(GUID);
     return ValueInfo(IsAnalysis, I == GlobalValueMap.end() ? nullptr : &*I);
   }
 
   /// Return a ValueInfo for \p GUID.
   ValueInfo getOrInsertValueInfo(GlobalValue::GUID GUID) {

include/llvm/LTO/LTO.h

 ///
 /// 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<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
-        recordNewLinkage);
+        recordNewLinkage,
+    function_ref<void(GlobalValue::GUID)> recordAutoHide);
 
 /// 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);
 

include/llvm/LTO/LTOBackend.h

Error backend(Config &C, AddStreamFn AddStream,
               unsigned ParallelCodeGenParallelismLevel,
               std::unique_ptr<Module> M, ModuleSummaryIndex &CombinedIndex);
 
 /// Runs a ThinLTO backend.
 Error thinBackend(Config &C, unsigned Task, AddStreamFn AddStream, Module &M,
                   const ModuleSummaryIndex &CombinedIndex,
                   const FunctionImporter::ImportMapTy &ImportList,
                   const GVSummaryMapTy &DefinedGlobals,
-                  MapVector<StringRef, BitcodeModule> &ModuleMap);
+                  MapVector<StringRef, BitcodeModule> &ModuleMap,
+                  const DenseSet<GlobalValue::GUID> &AutoHideCandidates);
 }
 }
 
 #endif

include/llvm/Transforms/IPO/FunctionImport.h

 
 /// Emit into \p OutputFilename the files module \p ModulePath will import from.
 std::error_code
 EmitImportsFiles(StringRef ModulePath, StringRef OutputFilename,
                  const FunctionImporter::ImportMapTy &ModuleImports);
 
 /// Resolve WeakForLinker values in \p TheModule based on the information
 /// recorded in the summaries during global summary-based analysis.
-void thinLTOResolveWeakForLinkerModule(Module &TheModule,
-                                       const GVSummaryMapTy &DefinedGlobals);
+void thinLTOResolveWeakForLinkerModule(
+    Module &TheModule, const GVSummaryMapTy &DefinedGlobals,
+    const DenseSet<GlobalValue::GUID> &AutoHideCandidates);
 
 /// Internalize \p TheModule based on the information recorded in the summaries
 /// during global summary-based analysis.
 void thinLTOInternalizeModule(Module &TheModule,
                               const GVSummaryMapTy &DefinedGlobals);
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_IPO_FUNCTIONIMPORT_H

lib/Analysis/ModuleSummaryAnalysis.cpp

computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
   bool NonRenamableLocal = isNonRenamableLocal(F);
   bool NotEligibleForImport =
       NonRenamableLocal || HasInlineAsmMaybeReferencingInternal ||
       // Inliner doesn't handle variadic functions.
       // FIXME: refactor this to use the same code that inliner is using.
       F.isVarArg() ||
       // Don't try to import functions with noinline attribute.
       F.getAttributes().hasFnAttribute(Attribute::NoInline);
-  GlobalValueSummary::GVFlags Flags(F.getLinkage(), NotEligibleForImport,
-                                    /* Live = */ false, F.isDSOLocal(),
-                                    /* UnnamedAddr= */false);
+  GlobalValueSummary::GVFlags Flags(
+      F.getLinkage(), NotEligibleForImport,
+      /* UnnamedAddr= */ F.hasAtLeastLocalUnnamedAddr());
   FunctionSummary::FFlags FunFlags{
       F.hasFnAttribute(Attribute::ReadNone),
       F.hasFnAttribute(Attribute::ReadOnly),
       F.hasFnAttribute(Attribute::NoRecurse),
       F.returnDoesNotAlias(),
   };
   auto FuncSummary = llvm::make_unique<FunctionSummary>(
       Flags, NumInsts, FunFlags, RefEdges.takeVector(),
       CallGraphEdges.takeVector(), TypeTests.takeVector(),
       TypeTestAssumeVCalls.takeVector(), TypeCheckedLoadVCalls.takeVector(),
       TypeTestAssumeConstVCalls.takeVector(),
       TypeCheckedLoadConstVCalls.takeVector());
   if (NonRenamableLocal)
     CantBePromoted.insert(F.getGUID());
   Index.addGlobalValueSummary(F.getName(), std::move(FuncSummary));
 }
 
 static void
 computeVariableSummary(ModuleSummaryIndex &Index, const GlobalVariable &V,
                        DenseSet<GlobalValue::GUID> &CantBePromoted) {
   SetVector<ValueInfo> RefEdges;
   SmallPtrSet<const User *, 8> Visited;
   findRefEdges(Index, &V, RefEdges, Visited);
   bool NonRenamableLocal = isNonRenamableLocal(V);
-  GlobalValueSummary::GVFlags Flags(V.getLinkage(), NonRenamableLocal,
-                                    /* Live = */ false, V.isDSOLocal(),
-                                    /* UnnamedAddr= */false);
+  GlobalValueSummary::GVFlags Flags(
+      V.getLinkage(), NonRenamableLocal,
+      /* UnnamedAddr= */ V.hasAtLeastLocalUnnamedAddr());
   auto GVarSummary =
       llvm::make_unique<GlobalVarSummary>(Flags, RefEdges.takeVector());
   if (NonRenamableLocal)
     CantBePromoted.insert(V.getGUID());
   Index.addGlobalValueSummary(V.getName(), std::move(GVarSummary));
 }
 
 static void
 computeAliasSummary(ModuleSummaryIndex &Index, const GlobalAlias &A,
                     DenseSet<GlobalValue::GUID> &CantBePromoted) {
   bool NonRenamableLocal = isNonRenamableLocal(A);
-  GlobalValueSummary::GVFlags Flags(A.getLinkage(), NonRenamableLocal,
-                                    /* Live = */ false, A.isDSOLocal(),
-                                    /* UnnamedAddr= */false);
+  GlobalValueSummary::GVFlags Flags(
+      A.getLinkage(), NonRenamableLocal,
+      /* UnnamedAddr= */ A.hasAtLeastLocalUnnamedAddr());
   auto AS = llvm::make_unique<AliasSummary>(Flags);
   auto *Aliasee = A.getBaseObject();
   auto *AliaseeSummary = Index.getGlobalValueSummary(*Aliasee);
   assert(AliaseeSummary && "Alias expects aliasee summary to be parsed");
   AS->setAliasee(AliaseeSummary);
   if (NonRenamableLocal)
     CantBePromoted.insert(A.getGUID());
   Index.addGlobalValueSummary(A.getName(), std::move(AS));
ModuleSymbolTable::CollectAsmSymbols(
           if (Flags & (object::BasicSymbolRef::SF_Weak |
                        object::BasicSymbolRef::SF_Global))
             return;
           HasLocalInlineAsmSymbol = true;
           GlobalValue *GV = M.getNamedValue(Name);
           if (!GV)
             return;
           assert(GV->isDeclaration() && "Def in module asm already has definition");
-          GlobalValueSummary::GVFlags GVFlags(GlobalValue::InternalLinkage,
-                                              /* NotEligibleToImport = */ true,
-                                              /* Live = */ true,
-                                              /* Local */ GV->isDSOLocal(),
-                                              /* UnnamedAddr= */ false);
+          GlobalValueSummary::GVFlags GVFlags(
+              GlobalValue::InternalLinkage,
+              /* UnnamedAddr= */ GV->hasAtLeastLocalUnnamedAddr());
           CantBePromoted.insert(GlobalValue::getGUID(Name));
           // Create the appropriate summary type.
           if (Function *F = dyn_cast<Function>(GV)) {
             std::unique_ptr<FunctionSummary> Summary =
                 llvm::make_unique<FunctionSummary>(
                     GVFlags, 0,
                     FunctionSummary::FFlags{
                         F->hasFnAttribute(Attribute::ReadNone),

lib/IR/ModuleSummaryIndex.cpp

bool ModuleSummaryIndex::isGUIDLive(GlobalValue::GUID GUID) const {
   if (SummaryList.empty())
     return true;
   for (auto &I : SummaryList)
     if (isGlobalValueLive(I.get()))
       return true;
   return false;
 }
 
+bool ModuleSummaryIndex::canGUIDAutoHide(GlobalValue::GUID GUID) const {
+  auto VI = getValueInfo(GUID);
+  if (!VI)
+    return false;
+  const auto &SummaryList = VI.getSummaryList();
+  if (SummaryList.empty())
+    return false;
+  bool canBeAutoHide = true;
+  for (auto &I : SummaryList)
+    canBeAutoHide &= I->flags().UnnamedAddr &&
+                     I->linkage() == GlobalValue::LinkOnceODRLinkage;
+  return canBeAutoHide;
+}
+
 namespace {
 struct Attributes {
   void add(const Twine &Name, const Twine &Value,
            const Twine &Comment = Twine());
   std::string getAsString() const;
 
   std::vector<std::string> Attrs;
   std::string Comments;

lib/LTO/LTO.cpp

 // 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 llvm::thinLTOResolveWeakForLinkerInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
-        recordNewLinkage) {
+        recordNewLinkage,
+    function_ref<void(GlobalValue::GUID)> recordAutoHide) {
   // 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.SummaryList)
       if (auto AS = dyn_cast<AliasSummary>(S.get()))
         GlobalInvolvedWithAlias.insert(&AS->getAliasee());
 
+  // Record the symbols that can be auto hide. This needs to be completed
+  // before resolve linkage.
+  for (auto &I : Index) {
+    if (Index.canGUIDAutoHide(I.first))
+      recordAutoHide(I.first);
+  }
+
   for (auto &I : Index)
     thinLTOResolveWeakForLinkerGUID(I.second.SummaryList, I.first,
                                     GlobalInvolvedWithAlias, isPrevailing,
                                     recordNewLinkage);
 }
 
 static void thinLTOInternalizeAndPromoteGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
ThinBackendProc(Config &Conf, ModuleSummaryIndex &CombinedIndex,
         ModuleToDefinedGVSummaries(ModuleToDefinedGVSummaries) {}
 
   virtual ~ThinBackendProc() {}
   virtual Error start(
       unsigned Task, BitcodeModule BM,
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
-      MapVector<StringRef, BitcodeModule> &ModuleMap) = 0;
+      MapVector<StringRef, BitcodeModule> &ModuleMap,
+      const DenseSet<GlobalValue::GUID> &AutoHideCandidates) = 0;
   virtual Error wait() = 0;
 };
 
 namespace {
 class InProcessThinBackend : public ThinBackendProc {
   ThreadPool BackendThreadPool;
   AddStreamFn AddStream;
   NativeObjectCache Cache;
public:
   Error runThinLTOBackendThread(
       AddStreamFn AddStream, NativeObjectCache Cache, unsigned Task,
       BitcodeModule BM, ModuleSummaryIndex &CombinedIndex,
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
       const GVSummaryMapTy &DefinedGlobals,
       MapVector<StringRef, BitcodeModule> &ModuleMap,
-      const TypeIdSummariesByGuidTy &TypeIdSummariesByGuid) {
+      const TypeIdSummariesByGuidTy &TypeIdSummariesByGuid,
+      const DenseSet<GlobalValue::GUID> &AutoHideCandidates) {
     auto RunThinBackend = [&](AddStreamFn AddStream) {
       LTOLLVMContext BackendContext(Conf);
       Expected<std::unique_ptr<Module>> MOrErr = BM.parseModule(BackendContext);
       if (!MOrErr)
         return MOrErr.takeError();
 
       return thinBackend(Conf, Task, AddStream, **MOrErr, CombinedIndex,
-                         ImportList, DefinedGlobals, ModuleMap);
+                         ImportList, DefinedGlobals, ModuleMap,
+                         AutoHideCandidates);
     };
 
     auto ModuleID = BM.getModuleIdentifier();
 
     if (!Cache || !CombinedIndex.modulePaths().count(ModuleID) ||
         all_of(CombinedIndex.getModuleHash(ModuleID),
                [](uint32_t V) { return V == 0; }))
       // Cache disabled or no entry for this module in the combined index or
Error runThinLTOBackendThread(
     return Error::success();
   }
 
   Error start(
       unsigned Task, BitcodeModule BM,
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
-      MapVector<StringRef, BitcodeModule> &ModuleMap) override {
+      MapVector<StringRef, BitcodeModule> &ModuleMap,
+      const DenseSet<GlobalValue::GUID> &AutoHideCandidates) override {
     StringRef ModulePath = BM.getModuleIdentifier();
     assert(ModuleToDefinedGVSummaries.count(ModulePath));
     const GVSummaryMapTy &DefinedGlobals =
         ModuleToDefinedGVSummaries.find(ModulePath)->second;
     BackendThreadPool.async(
         [=](BitcodeModule BM, ModuleSummaryIndex &CombinedIndex,
             const FunctionImporter::ImportMapTy &ImportList,
             const FunctionImporter::ExportSetTy &ExportList,
             const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>
                 &ResolvedODR,
             const GVSummaryMapTy &DefinedGlobals,
             MapVector<StringRef, BitcodeModule> &ModuleMap,
             const TypeIdSummariesByGuidTy &TypeIdSummariesByGuid) {
           Error E = runThinLTOBackendThread(
               AddStream, Cache, Task, BM, CombinedIndex, ImportList, ExportList,
-              ResolvedODR, DefinedGlobals, ModuleMap, TypeIdSummariesByGuid);
+              ResolvedODR, DefinedGlobals, ModuleMap, TypeIdSummariesByGuid,
+              AutoHideCandidates);
           if (E) {
             std::unique_lock<std::mutex> L(ErrMu);
             if (Err)
               Err = joinErrors(std::move(*Err), std::move(E));
             else
               Err = std::move(E);
           }
         },
WriteIndexesThinBackend(
         ShouldEmitImportsFiles(ShouldEmitImportsFiles),
         LinkedObjectsFileName(LinkedObjectsFileName), OnWrite(OnWrite) {}
 
   Error start(
       unsigned Task, BitcodeModule BM,
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
-      MapVector<StringRef, BitcodeModule> &ModuleMap) override {
+      MapVector<StringRef, BitcodeModule> &ModuleMap,
+      const DenseSet<GlobalValue::GUID> &AutoHideCandidates) override {
     StringRef ModulePath = BM.getModuleIdentifier();
     std::string NewModulePath =
         getThinLTOOutputFile(ModulePath, OldPrefix, NewPrefix);
 
     std::error_code EC;
     if (!LinkedObjectsFileName.empty()) {
       if (!LinkedObjectsFile) {
         LinkedObjectsFile = llvm::make_unique<raw_fd_ostream>(
auto isPrevailing = [&](GlobalValue::GUID GUID,
                           const GlobalValueSummary *S) {
     return ThinLTO.PrevailingModuleForGUID[GUID] == S->modulePath();
   };
   auto recordNewLinkage = [&](StringRef ModuleIdentifier,
                               GlobalValue::GUID GUID,
                               GlobalValue::LinkageTypes NewLinkage) {
     ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
   };
+  DenseSet<GlobalValue::GUID> AutoHideCandidates;
+  auto recordAutoHide = [&](GlobalValue::GUID GUID) {
+    AutoHideCandidates.insert(GUID);
+  };
+
   thinLTOResolveWeakForLinkerInIndex(ThinLTO.CombinedIndex, isPrevailing,
-                                     recordNewLinkage);
+                                     recordNewLinkage, recordAutoHide);
 
   std::unique_ptr<ThinBackendProc> BackendProc =
       ThinLTO.Backend(Conf, ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
                       AddStream, Cache);
 
   // Tasks 0 through ParallelCodeGenParallelismLevel-1 are reserved for combined
   // module and parallel code generation partitions.
   unsigned Task = RegularLTO.ParallelCodeGenParallelismLevel;
   for (auto &Mod : ThinLTO.ModuleMap) {
-    if (Error E = BackendProc->start(Task, Mod.second, ImportLists[Mod.first],
-                                     ExportLists[Mod.first],
-                                     ResolvedODR[Mod.first], ThinLTO.ModuleMap))
+    if (Error E = BackendProc->start(
+            Task, Mod.second, ImportLists[Mod.first], ExportLists[Mod.first],
+            ResolvedODR[Mod.first], ThinLTO.ModuleMap, AutoHideCandidates))
       return E;
     ++Task;
   }
 
   return BackendProc->wait();
 }
 
 Expected<std::unique_ptr<ToolOutputFile>>

lib/LTO/LTOBackend.cpp

if (GV->use_empty())
       GV->eraseFromParent();
   }
 }
 
 Error lto::thinBackend(Config &Conf, unsigned Task, AddStreamFn AddStream,
                        Module &Mod, const ModuleSummaryIndex &CombinedIndex,
                        const FunctionImporter::ImportMapTy &ImportList,
                        const GVSummaryMapTy &DefinedGlobals,
-                       MapVector<StringRef, BitcodeModule> &ModuleMap) {
+                       MapVector<StringRef, BitcodeModule> &ModuleMap,
+                       const DenseSet<GlobalValue::GUID> &AutoHideCandidates) {
   Expected<const Target *> TOrErr = initAndLookupTarget(Conf, Mod);
   if (!TOrErr)
     return TOrErr.takeError();
 
   std::unique_ptr<TargetMachine> TM = createTargetMachine(Conf, *TOrErr, Mod);
 
   if (Conf.CodeGenOnly) {
     codegen(Conf, TM.get(), AddStream, Task, Mod);
     return Error::success();
   }
 
   if (Conf.PreOptModuleHook && !Conf.PreOptModuleHook(Task, Mod))
     return Error::success();
 
   renameModuleForThinLTO(Mod, CombinedIndex);
 
   dropDeadSymbols(Mod, DefinedGlobals, CombinedIndex);
 
-  thinLTOResolveWeakForLinkerModule(Mod, DefinedGlobals);
+  thinLTOResolveWeakForLinkerModule(Mod, DefinedGlobals, AutoHideCandidates);
 
   if (Conf.PostPromoteModuleHook && !Conf.PostPromoteModuleHook(Task, Mod))
     return Error::success();
 
   if (!DefinedGlobals.empty())
     thinLTOInternalizeModule(Mod, DefinedGlobals);
 
   if (Conf.PostInternalizeModuleHook &&

lib/LTO/ThinLTOCodeGenerator.cpp

 };
 
 static std::unique_ptr<MemoryBuffer>
 ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
                      StringMap<MemoryBufferRef> &ModuleMap, TargetMachine &TM,
                      const FunctionImporter::ImportMapTy &ImportList,
                      const FunctionImporter::ExportSetTy &ExportList,
                      const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
+                     const DenseSet<GlobalValue::GUID> &AutoHideCandidates,
                      const GVSummaryMapTy &DefinedGlobals,
                      const ThinLTOCodeGenerator::CachingOptions &CacheOptions,
                      bool DisableCodeGen, StringRef SaveTempsDir,
                      bool Freestanding, unsigned OptLevel, unsigned count) {
 
   // "Benchmark"-like optimization: single-source case
   bool SingleModule = (ModuleMap.size() == 1);
 
   if (!SingleModule) {
     promoteModule(TheModule, Index);
 
     // Apply summary-based LinkOnce/Weak resolution decisions.
-    thinLTOResolveWeakForLinkerModule(TheModule, DefinedGlobals);
+    thinLTOResolveWeakForLinkerModule(TheModule, DefinedGlobals,
+                                      AutoHideCandidates);
 
     // Save temps: after promotion.
     saveTempBitcode(TheModule, SaveTempsDir, count, ".1.promoted.bc");
   }
 
   // Be friendly and don't nuke totally the module when the client didn't
   // supply anything to preserve.
   if (!ExportList.empty() || !GUIDPreservedSymbols.empty()) {

 /// 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,
     StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>>
-        &ResolvedODR) {
-
+        &ResolvedODR,
+    DenseSet<GlobalValue::GUID> &AutoHideCandidates) {
   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 recordNewLinkage = [&](StringRef ModuleIdentifier,
                               GlobalValue::GUID GUID,
                               GlobalValue::LinkageTypes NewLinkage) {
     ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
   };
 
-  thinLTOResolveWeakForLinkerInIndex(Index, isPrevailing, recordNewLinkage);
+  auto recordAutoHide = [&](GlobalValue::GUID GUID) {
+    AutoHideCandidates.insert(GUID);
+  };
+
+  thinLTOResolveWeakForLinkerInIndex(Index, isPrevailing, recordNewLinkage,
+                                     recordAutoHide);
 }
 
 // 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,
   // 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);
+  DenseSet<GlobalValue::GUID> AutoHideCandidates;
+  resolveWeakForLinkerInIndex(Index, ResolvedODR, AutoHideCandidates);
 
   thinLTOResolveWeakForLinkerModule(
-      TheModule, ModuleToDefinedGVSummaries[ModuleIdentifier]);
+      TheModule, ModuleToDefinedGVSummaries[ModuleIdentifier],
+      AutoHideCandidates);
 
   // Promote the exported values in the index, so that they are promoted
   // in the module.
   internalizeAndPromoteInIndex(ExportLists, GUIDPreservedSymbols, Index);
 
   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, ResolvedODR);
+  DenseSet<GlobalValue::GUID> AutoHideCandidates;
+  resolveWeakForLinkerInIndex(*Index, ResolvedODR, AutoHideCandidates);
 
   // 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.
   internalizeAndPromoteInIndex(ExportLists, GUIDPreservedSymbols, *Index);
 
   // Make sure that every module has an entry in the ExportLists and
   // ResolvedODR maps to enable threaded access to these maps below.
for (auto IndexCount : ModulesOrdering) {
 
         // Save temps: original file.
         saveTempBitcode(*TheModule, SaveTempsDir, count, ".0.original.bc");
 
         auto &ImportList = ImportLists[ModuleIdentifier];
         // Run the main process now, and generates a binary
         auto OutputBuffer = ProcessThinLTOModule(
             *TheModule, *Index, ModuleMap, *TMBuilder.create(), ImportList,
-            ExportList, GUIDPreservedSymbols,
+            ExportList, GUIDPreservedSymbols, AutoHideCandidates,
             ModuleToDefinedGVSummaries[ModuleIdentifier], CacheOptions,
             DisableCodeGen, SaveTempsDir, Freestanding, OptLevel, count);
 
         // Commit to the cache (if enabled)
         CacheEntry.write(*OutputBuffer);
 
         if (SavedObjectsDirectoryPath.empty()) {
           // We need to generated a memory buffer for the linker.

lib/Transforms/IPO/FunctionImport.cpp

if (Function *F = dyn_cast<Function>(&GV)) {
     GV.replaceAllUsesWith(NewGV);
     return false;
   }
   return true;
 }
 
 /// Fixup WeakForLinker linkages in \p TheModule based on summary analysis.
 void llvm::thinLTOResolveWeakForLinkerModule(
-    Module &TheModule, const GVSummaryMapTy &DefinedGlobals) {
+    Module &TheModule, const GVSummaryMapTy &DefinedGlobals,
+    const DenseSet<GlobalValue::GUID> &AutoHideCandidates) {
   auto updateLinkage = [&](GlobalValue &GV) {
     // See if the global summary analysis computed a new resolved linkage.
     const auto &GS = DefinedGlobals.find(GV.getGUID());
     if (GS == DefinedGlobals.end())
       return;
     auto NewLinkage = GS->second->linkage();
     if (NewLinkage == GV.getLinkage())
       return;
auto updateLinkage = [&](GlobalValue &GV) {
     if (GlobalValue::isAvailableExternallyLinkage(NewLinkage) &&
         GlobalValue::isInterposableLinkage(GV.getLinkage())) {
       if (!convertToDeclaration(GV))
         // FIXME: Change this to collect replaced GVs and later erase
         // them from the parent module once thinLTOResolveWeakForLinkerGUID is
         // changed to enable this for aliases.
         llvm_unreachable("Expected GV to be converted");
     } else {
-      // If the original symbols has global unnamed addr and linkonce_odr linkage,
-      // it should be an auto hide symbol. Add hidden visibility to the symbol to
-      // preserve the property.
-      if (GV.hasLinkOnceODRLinkage() && GV.hasGlobalUnnamedAddr() &&
-          NewLinkage == GlobalValue::WeakODRLinkage)
+      // If linkonce_odr symbols are promoted to weak_odr, make sure we don't
+      // lose the auto hide property. If linkonce_odr symbols has unnamed_addr
+      // or it is a candidate of auto hide, add hidden visibility to the symbol.
+      if ((GV.hasLinkOnceODRLinkage() &&
+           NewLinkage == GlobalValue::WeakODRLinkage) && // Is promote to weak.
+          (GV.hasGlobalUnnamedAddr() || AutoHideCandidates.count(GV.getGUID())))
         GV.setVisibility(GlobalValue::HiddenVisibility);
 
       DEBUG(dbgs() << "ODR fixing up linkage for `" << GV.getName() << "` from "
                    << GV.getLinkage() << " to " << NewLinkage << "\n");
       GV.setLinkage(NewLinkage);
     }
     // Remove declarations from comdats, including available_externally
     // as this is a declaration for the linker, and will be dropped eventually.

test/ThinLTO/X86/Inputs/linkonce_odr_unnamed_addr.ll

+@linkonceodrunnamed = linkonce_odr unnamed_addr constant i32 0
+
+@linkonceodrunnamed_local = linkonce_odr local_unnamed_addr constant i32 0
+
+@linkonceodrunnamed_m = linkonce_odr constant i32 0
+
+@linkonceodrunnamed_n = linkonce_odr unnamed_addr constant i32 0

test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll

 ; This test ensures that when linkonce_odr + unnamed_addr symbols promoted to
 ; weak symbols, it preserves the auto hide property.
 
 ; RUN: opt -module-summary %s -o %t.bc
-; RUN: opt -module-summary %s -o %t2.bc
+; RUN: opt -module-summary %p/Inputs/linkonce_odr_unnamed_addr.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
 ; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s
 
 ; CHECK: @linkonceodrunnamed = weak_odr hidden unnamed_addr constant i32 0
 @linkonceodrunnamed = linkonce_odr unnamed_addr constant i32 0
+
+; CHECK: @linkonceodrunnamed_local = weak_odr hidden local_unnamed_addr constant i32 0
+@linkonceodrunnamed_local = linkonce_odr local_unnamed_addr constant i32 0
+
+; CHECK: @linkonceodrunnamed_m = weak_odr local_unnamed_addr constant i32 0
+@linkonceodrunnamed_m = linkonce_odr local_unnamed_addr constant i32 0
+
+; CHECK: @linkonceodrunnamed_n = weak_odr hidden local_unnamed_addr constant i32 0
+@linkonceodrunnamed_n = linkonce_odr local_unnamed_addr constant i32 0