[ThinLTO] Add an auto-hide feature

[mehdi_amini]

Summary

when a symbol is not exported outside of the
DSO, it is can be hidden. Usually we try to internalize
as much as possible, but it is not always possible, for
instance a symbol can be referenced outside of the LTO
unit, or there can be cross-module reference in ThinLTO.

I'm still testing this patch, it may a bit rough around
the edges right now.

[mehdi_amini]

llvm/include/llvm/LTO/LTO.h

60 ↗

(On Diff #85233)

This is a good starting point to understand this patch.

[tejohnson]

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

980 ↗

(On Diff #85233)

Move this up and << 2 (consistent with LiveRoot and other bool flag setup)

llvm/lib/LTO/LTO.cpp

910 ↗

(On Diff #85233)

Add TODO about distinguishing which are used in regular LTO partition vs outside LTO/IR.

llvm/lib/LTO/ThinLTOCodeGenerator.cpp

900 ↗

(On Diff #85233)

Consider renaming this method

llvm/test/ThinLTO/X86/weak_autohide.ll

4 ↗

(On Diff #85233)

Unused

[pcc]

Does this need to be in the summary? I would have thought that the linker can use canBeOmittedFromSymbolTable() to implement its own auto-hiding logic. That is what ELF lld currently does.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

980 ↗

(On Diff #85233)

It looks like we are setting this variable in a very confusing way. I'd move the linkage first and then shift NotEligibleToImport and LiveRoot by 4 more.

[mehdi_amini]

In D28978#660963, @pcc wrote:

Does this need to be in the summary?

The part that is in the summary is not set in the compiler phase, but after the think-link, in order to communicate the *result* of the auto-hide analysis to the parallel backends.

I would have thought that the linker can use canBeOmittedFromSymbolTable() to implement its own auto-hiding logic. That is what ELF lld currently does.

That's kind of what this patch is doing :)

Especially we're using Res.VisibleToRegularObj as an input information, and this is set in gold for example there:

case LDPR_PREVAILING_DEF_IRONLY_EXP:
  R.Prevailing = true;
  if (!Res.CanOmitFromDynSym)
    R.VisibleToRegularObj = true;
  break;

And the Res.CanOmitFromDynSym is set this way: Res.CanOmitFromDynSym &= Sym.canBeOmittedFromSymbolTable();.

I haven't checked Gold, but I think you get the point: this is only using the symbol resolution API and no "side-information" from the summary.

That said, I'm not totally happy by the resolution API right now, and especially how we derived the partition. We don't have all the information we want I believe.
I discussed it this morning with Teresa and what I think my current frustration is with the lack of precise enough lattice in the partitions resolution. I think we should be able to get this:

External 
      | > Regular Object
      | > Outside of the DSO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

To get this, I believe right now we need one mode field in the symbol resolution API (example: unsigned VisibleOutsideDSO : 1) to distinguish between the two first cases,

[pcc]

In D28978#661021, @mehdi_amini wrote:

In D28978#660963, @pcc wrote:

Does this need to be in the summary?

The part that is in the summary is not set in the compiler phase, but after the think-link, in order to communicate the *result* of the auto-hide analysis to the parallel backends.

Oh I see. So you're communicating VisibleToRegularObj and information about the partitioning to the backends. Seems reasonable to me.

I would have thought that the linker can use canBeOmittedFromSymbolTable() to implement its own auto-hiding logic. That is what ELF lld currently does.

That's kind of what this patch is doing :)

Especially we're using Res.VisibleToRegularObj as an input information, and this is set in gold for example there:

case LDPR_PREVAILING_DEF_IRONLY_EXP:
  R.Prevailing = true;
  if (!Res.CanOmitFromDynSym)
    R.VisibleToRegularObj = true;
  break;

And the Res.CanOmitFromDynSym is set this way: Res.CanOmitFromDynSym &= Sym.canBeOmittedFromSymbolTable();.

I haven't checked Gold, but I think you get the point: this is only using the symbol resolution API and no "side-information" from the summary.

That said, I'm not totally happy by the resolution API right now, and especially how we derived the partition. We don't have all the information we want I believe.
I discussed it this morning with Teresa and what I think my current frustration is with the lack of precise enough lattice in the partitions resolution. I think we should be able to get this:

External 
      | > Regular Object
      | > Outside of the DSO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

To get this, I believe right now we need one mode field in the symbol resolution API (example: unsigned VisibleOutsideDSO : 1) to distinguish between the two first cases,

As we discussed on IRC, I don't think that's necessary. The linker can resolve this and decide on its own whether to add the symbol to the dynamic symbol table.

[mehdi_amini]

In D28978#661102, @pcc wrote:

To get this, I believe right now we need one mode field in the symbol resolution API (example: unsigned VisibleOutsideDSO : 1) to distinguish between the two first cases,

As we discussed on IRC, I don't think that's necessary. The linker can resolve this and decide on its own whether to add the symbol to the dynamic symbol table.

You're right, we can't do any optimization or any better codegen based on this information.

I still think we should refine the partitioning detection, since only the first two cases would be merged.

External 
      | > Outside of LTO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

What is introduced that we don't have today is a PartitionLTO and a PartitionThinLTO. This avoids going up to External immediately when a symbol is used in multiples individual ThinLTO modules, or when it is used across Regular and ThinLTO.

Right new we have to reconstruct this information in ThinLTO because we're losing it when constructing the partitioning.

[pcc]

In D28978#661105, @mehdi_amini wrote:

In D28978#661102, @pcc wrote:

To get this, I believe right now we need one mode field in the symbol resolution API (example: unsigned VisibleOutsideDSO : 1) to distinguish between the two first cases,

As we discussed on IRC, I don't think that's necessary. The linker can resolve this and decide on its own whether to add the symbol to the dynamic symbol table.

You're right, we can't do any optimization or any better codegen based on this information.

I still think we should refine the partitioning detection, since only the first two cases would be merged.

External 
      | > Outside of LTO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

What is introduced that we don't have today is a PartitionLTO and a PartitionThinLTO. This avoids going up to External immediately when a symbol is used in multiples individual ThinLTO modules, or when it is used across Regular and ThinLTO.

Right new we have to reconstruct this information in ThinLTO because we're losing it when constructing the partitioning.

I'm not sure I understand how PartitionLTO would be different from !IsVisibleToRegularObj, or PartitionThinLTO from Partition == External && !VisibleOutsideThinLTO.

[mehdi_amini]

In D28978#661111, @pcc wrote:

In D28978#661105, @mehdi_amini wrote:

In D28978#661102, @pcc wrote:

To get this, I believe right now we need one mode field in the symbol resolution API (example: unsigned VisibleOutsideDSO : 1) to distinguish between the two first cases,

As we discussed on IRC, I don't think that's necessary. The linker can resolve this and decide on its own whether to add the symbol to the dynamic symbol table.

You're right, we can't do any optimization or any better codegen based on this information.

I still think we should refine the partitioning detection, since only the first two cases would be merged.

External 
      | > Outside of LTO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

What is introduced that we don't have today is a PartitionLTO and a PartitionThinLTO. This avoids going up to External immediately when a symbol is used in multiples individual ThinLTO modules, or when it is used across Regular and ThinLTO.

Right new we have to reconstruct this information in ThinLTO because we're losing it when constructing the partitioning.

I'm not sure I understand how PartitionLTO would be different from !IsVisibleToRegularObj, or PartitionThinLTO from Partition == External && !VisibleOutsideThinLTO.

It is not different, that's the point: VisibleOutsideThinLTO was added after the fact because the partition were losing the information. I think it was not a good addition and fixing the partitions instead was the way to go, Partition == External && !VisibleOutsideThinLTO is not something I'd like to read anywhere.
.

[pcc]

In D28978#661140, @mehdi_amini wrote:

In D28978#661111, @pcc wrote:

In D28978#661105, @mehdi_amini wrote:

In D28978#661102, @pcc wrote:

To get this, I believe right now we need one mode field in the symbol resolution API (example: unsigned VisibleOutsideDSO : 1) to distinguish between the two first cases,

As we discussed on IRC, I don't think that's necessary. The linker can resolve this and decide on its own whether to add the symbol to the dynamic symbol table.

You're right, we can't do any optimization or any better codegen based on this information.

I still think we should refine the partitioning detection, since only the first two cases would be merged.

External 
      | > Outside of LTO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

What is introduced that we don't have today is a PartitionLTO and a PartitionThinLTO. This avoids going up to External immediately when a symbol is used in multiples individual ThinLTO modules, or when it is used across Regular and ThinLTO.

Right new we have to reconstruct this information in ThinLTO because we're losing it when constructing the partitioning.

I'm not sure I understand how PartitionLTO would be different from !IsVisibleToRegularObj, or PartitionThinLTO from Partition == External && !VisibleOutsideThinLTO.

It is not different, that's the point: VisibleOutsideThinLTO was added after the fact because the partition were losing the information. I think it was not a good addition and fixing the partitions instead was the way to go, Partition == External && !VisibleOutsideThinLTO is not something I'd like to read anywhere.

Agreed that we should rethink partitions, it would be nice to have a better way to write that query.

[mehdi_amini]

Address comments

[mehdi_amini]

llvm/lib/LTO/LTO.cpp

910 ↗

(On Diff #85233)

After discussing with Peter, we believe this is something the native linker could do.

What is not done in the current patch but can help (a little) the Codegen is to propagate the FinalDefinitionInLinkageUnit, to every uses to skip the GOT.

[tejohnson]

LGTM with comment nit below.

llvm/lib/LTO/LTO.cpp

910 ↗

(On Diff #86847)

Nit: typos in comment:
s/hidding/hiding/
s/reference from/referenced from/

also maybe add "only" before "referenced from" to clarify.

[mehdi_amini]

FYI: after talking more with @pcc and doing more tests: this is not meaningful and a strict subset of what the linker can do (and already does).

I revert the patch.

[tejohnson]

FYI I also just confirmed that this patch was causing linker errors in SPEC 450.soplex of the form:

/usr/bin/ld: error: hidden symbol '_ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_PKc' is not defined locally
/usr/bin/ld: error: hidden symbol '_ZSt4endlIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_' is not defined locally

llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h

struct GVFlags {
     /// be renamed or references something that can't be renamed).
     unsigned NotEligibleToImport : 1;
 
     /// Indicate that the global value must be considered a live root for
     /// index-based liveness analysis. Used for special LLVM values such as
     /// llvm.global_ctors that the linker does not know about.
     unsigned LiveRoot : 1;
 
+    /// Indicate if the global value should be hidden.
+    unsigned AutoHide : 1;
+
     /// Convenience Constructors
     explicit GVFlags(GlobalValue::LinkageTypes Linkage,
-                     bool NotEligibleToImport, bool LiveRoot)
+                     bool NotEligibleToImport, bool LiveRoot, bool AutoHide)
         : Linkage(Linkage), NotEligibleToImport(NotEligibleToImport),
-          LiveRoot(LiveRoot) {}
+          LiveRoot(LiveRoot), AutoHide(AutoHide) {}
   };
 
 private:
   /// Kind of summary for use in dyn_cast<> et al.
   SummaryKind Kind;
 
   GVFlags Flags;
 
public:
   }
 
   /// Sets the linkage to the value determined by global summary-based
   /// optimization. Will be applied in the ThinLTO backends.
   void setLinkage(GlobalValue::LinkageTypes Linkage) {
     Flags.Linkage = Linkage;
   }
 
+  /// Sets the visibility to be autohidden.
+  void setAutoHide() { Flags.AutoHide = true; }
+
   /// Return true if this global value can't be imported.
   bool notEligibleToImport() const { return Flags.NotEligibleToImport; }
 
   /// Return true if this global value must be considered a root for live
   /// value analysis on the index.
   bool liveRoot() const { return Flags.LiveRoot; }
 
   /// Flag that this global value must be considered a root for live

llvm/trunk/include/llvm/IR/ModuleSummaryIndexYAML.h

static void inputOne(IO &io, StringRef Key, GlobalValueSummaryMapTy &V) {
     uint64_t KeyInt;
     if (Key.getAsInteger(0, KeyInt)) {
       io.setError("key not an integer");
       return;
     }
     auto &Elem = V[KeyInt];
     for (auto &FSum : FSums) {
       GlobalValueSummary::GVFlags GVFlags(GlobalValue::ExternalLinkage, false,
-                                          false);
+                                          false, /* AutoHide */ false);
       Elem.push_back(llvm::make_unique<FunctionSummary>(
           GVFlags, 0, ArrayRef<ValueInfo>{},
           ArrayRef<FunctionSummary::EdgeTy>{}, std::move(FSum.TypeTests)));
     }
   }
   static void output(IO &io, GlobalValueSummaryMapTy &V) {
     for (auto &P : V) {
       std::vector<FunctionSummaryYaml> FSums;

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

 /// 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);
 
+/// This enum is used for the returned value of the callback passed to
+/// thinLTOInternalizeAndPromoteInIndex, it indicates if a symbol can be made
+/// Internal (only referenced from its defining object), Hidden (
+/// outside the DSO), or Exported (exposed as public API for the DSO).
+enum SummaryResolution { Internal, Hidden, Exported };
+
 /// 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);
+    function_ref<SummaryResolution(StringRef, GlobalValue::GUID)> isExported);
 
 namespace lto {
 
 /// Given the original \p Path to an output file, replace any path
 /// prefix matching \p OldPrefix with \p NewPrefix. Also, create the
 /// resulting directory if it does not yet exist.
 std::string getThinLTOOutputFile(const std::string &Path,
                                  const std::string &OldPrefix,

llvm/trunk/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();
   GlobalValueSummary::GVFlags Flags(F.getLinkage(), NotEligibleForImport,
-                                    /* LiveRoot = */ false);
+                                    /* LiveRoot = */ false,
+                                    /* AutoHide */ false);
   auto FuncSummary = llvm::make_unique<FunctionSummary>(
       Flags, NumInsts, RefEdges.takeVector(), CallGraphEdges.takeVector(),
       TypeTests.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(&V, RefEdges, Visited);
   bool NonRenamableLocal = isNonRenamableLocal(V);
   GlobalValueSummary::GVFlags Flags(V.getLinkage(), NonRenamableLocal,
-                                    /* LiveRoot = */ false);
+                                    /* LiveRoot = */ false,
+                                    /* AutoHide */ false);
   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,
-                                    /* LiveRoot = */ false);
+                                    /* LiveRoot = */ false,
+                                    /* AutoHide */ false);
   auto AS = llvm::make_unique<AliasSummary>(Flags, ArrayRef<ValueInfo>{});
   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(
                        object::BasicSymbolRef::SF_Global))
             return;
           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,
-                                              /* LiveRoot */ true);
+                                              /* LiveRoot */ true,
+                                              /* AutoHide */ false);
           CantBePromoted.insert(GlobalValue::getGUID(Name));
           // Create the appropriate summary type.
           if (isa<Function>(GV)) {
             std::unique_ptr<FunctionSummary> Summary =
                 llvm::make_unique<FunctionSummary>(
                     GVFlags, 0, ArrayRef<ValueInfo>{},
                     ArrayRef<FunctionSummary::EdgeTy>{},
                     ArrayRef<GlobalValue::GUID>{});

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

 
 /// Decode the flags for GlobalValue in the summary.
 static GlobalValueSummary::GVFlags getDecodedGVSummaryFlags(uint64_t RawFlags,
                                                             uint64_t Version) {
   // Summary were not emitted before LLVM 3.9, we don't need to upgrade Linkage
   // like getDecodedLinkage() above. Any future change to the linkage enum and
   // to getDecodedLinkage() will need to be taken into account here as above.
   auto Linkage = GlobalValue::LinkageTypes(RawFlags & 0xF); // 4 bits
-  RawFlags = RawFlags >> 4;
-  bool NotEligibleToImport = (RawFlags & 0x1) || Version < 3;
+  bool NotEligibleToImport = ((RawFlags >> 4) & 0x1) || Version < 3;
   // The LiveRoot flag wasn't introduced until version 3. For dead stripping
   // to work correctly on earlier versions, we must conservatively treat all
   // values as live.
-  bool LiveRoot = (RawFlags & 0x2) || Version < 3;
-  return GlobalValueSummary::GVFlags(Linkage, NotEligibleToImport, LiveRoot);
+  bool LiveRoot = ((RawFlags >> 5) & 0x1) || Version < 3;
+  bool AutoHide = (RawFlags >> 6) & 0x1;
+  return GlobalValueSummary::GVFlags(Linkage, NotEligibleToImport, LiveRoot,
+                                     AutoHide);
 }
 
 static GlobalValue::VisibilityTypes getDecodedVisibility(unsigned Val) {
   switch (Val) {
   default: // Map unknown visibilities to default.
   case 0: return GlobalValue::DefaultVisibility;
   case 1: return GlobalValue::HiddenVisibility;
   case 2: return GlobalValue::ProtectedVisibility;

llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp

 static unsigned getEncodedLinkage(const GlobalValue &GV) {
   return getEncodedLinkage(GV.getLinkage());
 }
 
 // Decode the flags for GlobalValue in the summary
 static uint64_t getEncodedGVSummaryFlags(GlobalValueSummary::GVFlags Flags) {
   uint64_t RawFlags = 0;
 
-  RawFlags |= Flags.NotEligibleToImport; // bool
-  RawFlags |= (Flags.LiveRoot << 1);
   // Linkage don't need to be remapped at that time for the summary. Any future
   // change to the getEncodedLinkage() function will need to be taken into
   // account here as well.
-  RawFlags = (RawFlags << 4) | Flags.Linkage; // 4 bits
-
+  RawFlags |= Flags.Linkage;                    // 4 bits linkage
+  RawFlags |= (Flags.NotEligibleToImport << 4); // bool
+  RawFlags |= (Flags.LiveRoot << 5);            // bool
+  RawFlags |= (Flags.AutoHide << 6);            // bool
   return RawFlags;
 }
 
 static unsigned getEncodedVisibility(const GlobalValue &GV) {
   switch (GV.getVisibility()) {
   case GlobalValue::DefaultVisibility:   return 0;
   case GlobalValue::HiddenVisibility:    return 1;
   case GlobalValue::ProtectedVisibility: return 2;

llvm/trunk/lib/LTO/LTO.cpp

void llvm::thinLTOResolveWeakForLinkerInIndex(
 
   for (auto &I : Index)
     thinLTOResolveWeakForLinkerGUID(I.second, I.first, GlobalInvolvedWithAlias,
                                     isPrevailing, recordNewLinkage);
 }
 
 static void thinLTOInternalizeAndPromoteGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
-    function_ref<bool(StringRef, GlobalValue::GUID)> isExported) {
+    function_ref<SummaryResolution(StringRef, GlobalValue::GUID)> isExported) {
   for (auto &S : GVSummaryList) {
-    if (isExported(S->modulePath(), GUID)) {
+    auto ExportResolution = isExported(S->modulePath(), GUID);
+    if (ExportResolution != Internal) {
       if (GlobalValue::isLocalLinkage(S->linkage()))
         S->setLinkage(GlobalValue::ExternalLinkage);
+      if (ExportResolution == Hidden)
+        S->setAutoHide();
     } else if (!GlobalValue::isLocalLinkage(S->linkage()))
       S->setLinkage(GlobalValue::InternalLinkage);
   }
 }
 
 // Update the linkages in the given \p Index to mark exported values
 // as external and non-exported values as internal.
 void llvm::thinLTOInternalizeAndPromoteInIndex(
     ModuleSummaryIndex &Index,
-    function_ref<bool(StringRef, GlobalValue::GUID)> isExported) {
+    function_ref<SummaryResolution(StringRef, GlobalValue::GUID)> isExported) {
   for (auto &I : Index)
     thinLTOInternalizeAndPromoteGUID(I.second, I.first, isExported);
 }
 
 struct InputFile::InputModule {
   BitcodeModule BM;
   std::unique_ptr<Module> Mod;
 
for (auto &Res : GlobalResolutions) {
       if (!DeadSymbols.count(GUID))
         ExportedGUIDs.insert(GlobalValue::getGUID(Res.second.IRName));
     }
 
     auto isPrevailing = [&](GlobalValue::GUID GUID,
                             const GlobalValueSummary *S) {
       return ThinLTO.PrevailingModuleForGUID[GUID] == S->modulePath();
     };
-    auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
+    auto isExported = [&](StringRef ModuleIdentifier,
+                          GlobalValue::GUID GUID) -> SummaryResolution {
       const auto &ExportList = ExportLists.find(ModuleIdentifier);
-      return (ExportList != ExportLists.end() &&
-              ExportList->second.count(GUID)) ||
-             ExportedGUIDs.count(GUID);
+      if ((ExportList != ExportLists.end() && ExportList->second.count(GUID)) ||
+          ExportedGUIDs.count(GUID)) {
+        // We could do better by hiding when a symbol is in
+        // GUIDPreservedSymbols because it is only referenced from regular LTO
+        // or from native files and not outside the final binary, but that's
+        // something the native linker could do as gwell.
+        if (GUIDPreservedSymbols.count(GUID))
+          return Exported;
+        return Hidden;
+      }
+      return Internal;
     };
     thinLTOInternalizeAndPromoteInIndex(ThinLTO.CombinedIndex, isExported);
 
     auto recordNewLinkage = [&](StringRef ModuleIdentifier,
                                 GlobalValue::GUID GUID,
                                 GlobalValue::LinkageTypes NewLinkage) {
       ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
     };

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

static void optimizeModule(Module &TheModule, TargetMachine &TM,
   // Add optimizations
   PMB.populateThinLTOPassManager(PM);
 
   PM.run(TheModule);
 }
 
 // Convert the PreservedSymbols map from "Name" based to "GUID" based.
 static DenseSet<GlobalValue::GUID>
-computeGUIDPreservedSymbols(const StringSet<> &PreservedSymbols,
-                            const Triple &TheTriple) {
+convertSymbolNamesToGUID(const StringSet<> &NamedSymbols,
   DenseSet<GlobalValue::GUID> GUIDPreservedSymbols(PreservedSymbols.size());
   for (auto &Entry : PreservedSymbols) {
     StringRef Name = Entry.first();
     if (TheTriple.isOSBinFormatMachO() && Name.size() > 0 && Name[0] == '_')
       Name = Name.drop_front();
     GUIDPreservedSymbols.insert(GlobalValue::getGUID(Name));
   }
   return GUIDPreservedSymbols;
#endif
   Modules.push_back(Buffer);
 }
 
 void ThinLTOCodeGenerator::preserveSymbol(StringRef Name) {
   PreservedSymbols.insert(Name);
 }
 
 void ThinLTOCodeGenerator::crossReferenceSymbol(StringRef Name) {
-  // FIXME: At the moment, we don't take advantage of this extra information,
-  // we're conservatively considering cross-references as preserved.
-  //  CrossReferencedSymbols.insert(Name);
-  PreservedSymbols.insert(Name);
+  CrossReferencedSymbols.insert(Name);
 }
 
 // TargetMachine factory
 std::unique_ptr<TargetMachine> TargetMachineBuilder::create() const {
   std::string ErrMsg;
   const Target *TheTarget =
       TargetRegistry::lookupTarget(TheTriple.str(), ErrMsg);
   if (!TheTarget) {
void ThinLTOCodeGenerator::promote(Module &TheModule,
   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);
 
   // Convert the preserved symbols set from string to GUID
-  auto GUIDPreservedSymbols = computeGUIDPreservedSymbols(
+  auto GUIDPreservedSymbols = convertSymbolNamesToGUID(
       PreservedSymbols, Triple(TheModule.getTargetTriple()));
 
   // Compute "dead" symbols, we don't want to import/export these!
   auto DeadSymbols = computeDeadSymbols(Index, GUIDPreservedSymbols);
 
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists, &DeadSymbols);
 
   // Resolve LinkOnce/Weak symbols.
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
   resolveWeakForLinkerInIndex(Index, ResolvedODR);
 
   thinLTOResolveWeakForLinkerModule(
       TheModule, ModuleToDefinedGVSummaries[ModuleIdentifier]);
 
   // Promote the exported values in the index, so that they are promoted
   // in the module.
-  auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
+  auto isExported = [&](StringRef ModuleIdentifier,
+                        GlobalValue::GUID GUID) -> SummaryResolution {
     const auto &ExportList = ExportLists.find(ModuleIdentifier);
-    return (ExportList != ExportLists.end() &&
-            ExportList->second.count(GUID)) ||
-           GUIDPreservedSymbols.count(GUID);
+    if ((ExportList != ExportLists.end() && ExportList->second.count(GUID)) ||
+        GUIDPreservedSymbols.count(GUID))
+      return Exported;
+    return Internal;
   };
   thinLTOInternalizeAndPromoteInIndex(Index, isExported);
 
   promoteModule(TheModule, Index);
 }
 
 /**
  * Perform cross-module importing for the module identified by ModuleIdentifier.
  */
 void ThinLTOCodeGenerator::crossModuleImport(Module &TheModule,
                                              ModuleSummaryIndex &Index) {
   auto ModuleMap = generateModuleMap(Modules);
   auto ModuleCount = Index.modulePaths().size();
 
   // Collect for each module the list of function it defines (GUID -> Summary).
   StringMap<GVSummaryMapTy> ModuleToDefinedGVSummaries(ModuleCount);
   Index.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // Convert the preserved symbols set from string to GUID
-  auto GUIDPreservedSymbols = computeGUIDPreservedSymbols(
+  auto GUIDPreservedSymbols = convertSymbolNamesToGUID(
       PreservedSymbols, Triple(TheModule.getTargetTriple()));
 
   // Compute "dead" symbols, we don't want to import/export these!
   auto DeadSymbols = computeDeadSymbols(Index, GUIDPreservedSymbols);
 
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);

 void ThinLTOCodeGenerator::internalize(Module &TheModule,
                                        ModuleSummaryIndex &Index) {
   initTMBuilder(TMBuilder, Triple(TheModule.getTargetTriple()));
   auto ModuleCount = Index.modulePaths().size();
   auto ModuleIdentifier = TheModule.getModuleIdentifier();
 
   // Convert the preserved symbols set from string to GUID
   auto GUIDPreservedSymbols =
-      computeGUIDPreservedSymbols(PreservedSymbols, TMBuilder.TheTriple);
+      convertSymbolNamesToGUID(PreservedSymbols, TMBuilder.TheTriple);
 
   // Collect for each module the list of function it defines (GUID -> Summary).
   StringMap<GVSummaryMapTy> ModuleToDefinedGVSummaries(ModuleCount);
   Index.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // Compute "dead" symbols, we don't want to import/export these!
   auto DeadSymbols = computeDeadSymbols(Index, GUIDPreservedSymbols);
 
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists, &DeadSymbols);
   auto &ExportList = ExportLists[ModuleIdentifier];
 
   // Be friendly and don't nuke totally the module when the client didn't
   // supply anything to preserve.
   if (ExportList.empty() && GUIDPreservedSymbols.empty())
     return;
 
   // Internalization
-  auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
+  auto isExported = [&](StringRef ModuleIdentifier,
+                        GlobalValue::GUID GUID) -> SummaryResolution {
     const auto &ExportList = ExportLists.find(ModuleIdentifier);
-    return (ExportList != ExportLists.end() &&
-            ExportList->second.count(GUID)) ||
-           GUIDPreservedSymbols.count(GUID);
+    if ((ExportList != ExportLists.end() && ExportList->second.count(GUID)) ||
+        GUIDPreservedSymbols.count(GUID))
+      return Exported;
+    return Internal;
   };
   thinLTOInternalizeAndPromoteInIndex(Index, isExported);
   thinLTOInternalizeModule(TheModule,
                            ModuleToDefinedGVSummaries[ModuleIdentifier]);
 }
 
 /**
  * Perform post-importing ThinLTO optimizations.
void ThinLTOCodeGenerator::run() {
 
   // Collect for each module the list of function it defines (GUID -> Summary).
   StringMap<GVSummaryMapTy> ModuleToDefinedGVSummaries(ModuleCount);
   Index->collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // Convert the preserved symbols set from string to GUID, this is needed for
   // computing the caching hash and the internalization.
   auto GUIDPreservedSymbols =
-      computeGUIDPreservedSymbols(PreservedSymbols, TMBuilder.TheTriple);
+      convertSymbolNamesToGUID(PreservedSymbols, TMBuilder.TheTriple);
+  auto GUIDCrossRefSymbols =
+      convertSymbolNamesToGUID(CrossReferencedSymbols, TMBuilder.TheTriple);
 
   // Compute "dead" symbols, we don't want to import/export these!
   auto DeadSymbols = computeDeadSymbols(*Index, GUIDPreservedSymbols);
 
   // Collect the import/export lists for all modules from the call-graph in the
   // combined index.
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(*Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists, &DeadSymbols);
 
   // 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);
 
-  auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
+  auto isExported = [&](StringRef ModuleIdentifier,
+                        GlobalValue::GUID GUID) -> SummaryResolution {
+    if (GUIDPreservedSymbols.count(GUID))
+      return Exported;
+    if (GUIDCrossRefSymbols.count(GUID))
+      return Hidden;
     const auto &ExportList = ExportLists.find(ModuleIdentifier);
-    return (ExportList != ExportLists.end() &&
-            ExportList->second.count(GUID)) ||
-           GUIDPreservedSymbols.count(GUID);
+    if (ExportList != ExportLists.end() && ExportList->second.count(GUID))
+      return Hidden;
+    return Internal;
   };
 
   // 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(*Index, isExported);
 
   // Make sure that every module has an entry in the ExportLists and

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

if (GS == DefinedGlobals.end()) {
       } else {
         Linkage = GS->second->linkage();
       }
     } else
       Linkage = GS->second->linkage();
     return !GlobalValue::isLocalLinkage(Linkage);
   };
 
+  // Try to auto-hide the symbols.
+  for (auto &GO : TheModule.global_objects()) {
+    const auto &GS = DefinedGlobals.find(GO.getGUID());
+    if (GS != DefinedGlobals.end() && GS->second->flags().AutoHide)
+      GO.setVisibility(GlobalValue::HiddenVisibility);
+  }
+
   // FIXME: See if we can just internalize directly here via linkage changes
   // based on the index, rather than invoking internalizeModule.
   llvm::internalizeModule(TheModule, MustPreserveGV);
 }
 
 // Automatically import functions in Module \p DestModule based on the summaries
 // index.
 //

llvm/trunk/test/ThinLTO/X86/Inputs/weak_autohide.ll

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+define weak_odr void @weakodrfunc() {
+  ret void
+}

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

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %p/Inputs/deadstrip.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc %t2.bc
 
 ; RUN: llvm-lto -exported-symbol=_main -thinlto-action=promote %t1.bc -thinlto-index=%t.index.bc -o - | llvm-lto -exported-symbol=_main -thinlto-action=internalize -thinlto-index %t.index.bc -thinlto-module-id=%t1.bc - -o - | llvm-dis -o - | FileCheck %s
-; RUN: llvm-lto -exported-symbol=_main -thinlto-action=promote %t2.bc -thinlto-index=%t.index.bc -o - | llvm-lto -exported-symbol=_main -thinlto-action=internalize -thinlto-index %t.index.bc -thinlto-module-id=%t2.bc - -o - | llvm-dis -o - | FileCheck %s --check-prefix=CHECK2
+; RUN: llvm-lto -exported-symbol=_main -thinlto-action=promote %t2.bc -thinlto-index=%t.index.bc -o - | llvm-lto -exported-symbol=_main -thinlto-action=internalize -thinlto-index %t.index.bc -thinlto-module-id=%t2.bc - -o - | llvm-dis -o - | FileCheck %s --check-prefix=CHECK2-LTO
 
 ; RUN: llvm-lto -exported-symbol=_main -thinlto-action=run %t1.bc %t2.bc
 ; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s --check-prefix=CHECK-NM
 
 ; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.out -save-temps \
 ; RUN:   -r %t1.bc,_main,plx \
 ; RUN:   -r %t1.bc,_bar,pl \
 ; RUN:   -r %t1.bc,_dead_func,pl \
 ; RUN:   -r %t1.bc,_baz,l \
 ; RUN:   -r %t1.bc,_boo,l \
 ; RUN:   -r %t2.bc,_baz,pl \
 ; RUN:   -r %t2.bc,_boo,pl \
 ; RUN:   -r %t2.bc,_dead_func,pl \
 ; RUN:   -r %t2.bc,_another_dead_func,pl
 ; RUN: llvm-dis < %t.out.0.3.import.bc | FileCheck %s
-; RUN: llvm-dis < %t.out.1.3.import.bc | FileCheck %s --check-prefix=CHECK2
+; RUN: llvm-dis < %t.out.1.3.import.bc | FileCheck %s --check-prefix=CHECK2-LTO2
 ; RUN: llvm-nm %t.out.1 | FileCheck %s --check-prefix=CHECK2-NM
 
 ; Dead-stripping on the index allows to internalize these,
 ; and limit the import of @baz thanks to early pruning.
 ; CHECK-NOT: available_externally {{.*}} @baz()
 ; CHECK: @llvm.global_ctors =
 ; CHECK: define internal void @_GLOBAL__I_a()
 ; CHECK: define internal void @bar() {
 ; CHECK: define internal void @bar_internal()
 ; CHECK: define internal void @dead_func() {
 ; CHECK-NOT: available_externally {{.*}} @baz()
 
 ; Make sure we didn't internalize @boo, which is reachable via
 ; llvm.global_ctors
-; CHECK2: define void @boo()
+; CHECK2-LTO: define void @boo()
+; CHECK2-LTO2: define hidden void @boo()
 ; We should have eventually revoved @baz since it was internalized and unused
 ; CHECK2-NM-NOT: _baz
 
 ; The final binary should not contain any of the dead functions,
 ; only main is expected because bar is expected to be inlined and stripped out.
 ; CHECK-NM-NOT: bar
 ; CHECK-NM-NOT: dead
 ; CHECK-NM: T _main

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

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/weak_autohide.ll -o %t2.bc
+
+; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
+; RUN:     -r=%t1.bc,_strong_func,pxl \
+; RUN:     -r=%t1.bc,_weakodrfunc,pl \
+; RUN:     -r=%t2.bc,_weakodrfunc,l
+; RUN: llvm-dis < %t.o.0.2.internalize.bc | FileCheck  %s --check-prefix=AUTOHIDE
+
+
+; AUTOHIDE: weak_odr hidden void @weakodrfunc
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+define weak_odr void @weakodrfunc() #0 {
+  ret void
+}
+
+define void @strong_func() #0 {
+  call void @weakodrfunc()
+  ret void
+}
+