[LTO] Initialize canAutoHide() using canBeOmittedFromSymbolTable()

[int3]

Summary

Per discussion on
https://reviews.llvm.org/D59709#inline-1148734, this seems like the
right course of action. canBeOmittedFromSymbolTable() subsumes and
generalizes the previous logic. In addition to handling linkonce_odr
unnamed_addr globals, we now also internalize linkonce_odr +
local_unnamed_addr constants.

[int3]

format

[int3]

bump -- @tejohnson, could you take a look? :)

[tejohnson]

LGTM, thanks for fixing! One suggestion below for updating the test description. Also, can you update the patch description to note specifically what is changing as a result of the patch - i.e. that local_unnamed_addr constants can now also be auto hidden. The current summary mentions that the prior approach only handled linkonce_odr globals, which is still the case.

llvm/test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll

1

Update comment.

[int3]

update comments and commit msg

lld/test/MachO/lto-internalize-unnamed-addr.ll

 ; REQUIRES: x86
 ; RUN: rm -rf %t; split-file %s %t
 ;; This test covers both FullLTO and ThinLTO code paths because we have observed
 ;; (unexpected) differences between the two.
 ; RUN: llvm-as %t/test.ll -o %t/test.o
 ; RUN: llvm-as %t/test2.ll -o %t/test2.o
 ; RUN: opt -module-summary %t/test.ll -o %t/test.thinlto.o
 ; RUN: opt -module-summary %t/test2.ll -o %t/test2.thinlto.o
 
 ; RUN: %lld -lSystem %t/test.o %t/test2.o -o %t/test
 ; RUN: llvm-nm -m %t/test | FileCheck %s --check-prefix=LTO
 
 ; RUN: %lld -lSystem -dylib %t/test.o %t/test2.o -o %t/test.dylib
 ; RUN: llvm-nm -m %t/test.dylib | FileCheck %s --check-prefix=LTO-DYLIB
 
-; RUN: %lld -lSystem %t/test.thinlto.o %t/test2.o -o %t/test.thinlto
+; RUN: %lld -lSystem %t/test.thinlto.o %t/test2.thinlto.o -o %t/test.thinlto
 ; RUN: llvm-nm -m %t/test.thinlto | FileCheck %s --check-prefix=THINLTO
 
-; RUN: %lld -lSystem -dylib %t/test.thinlto.o %t/test2.o -o %t/test.thinlto.dylib
+; RUN: %lld -lSystem -dylib %t/test.thinlto.o %t/test2.thinlto.o -o %t/test.thinlto.dylib
 ; RUN: llvm-nm -m %t/test.thinlto.dylib | FileCheck %s --check-prefix=THINLTO
 
 ; LTO-DAG: (__DATA,__data) non-external _global_unnamed
 ; LTO-DAG: (__DATA,__data) non-external _local_unnamed
 ;; LD64 marks this with (was a private external). IMO both LD64 and LLD should
 ;; mark all the other internalized symbols with (was a private external).
 ; LTO-DAG: (__TEXT,__const) non-external _local_unnamed_always_const
 ; LTO-DAG: (__TEXT,__const) non-external _local_unnamed_const
 ;; LD64 doesn't internalize this -- it emits it as a weak external -- which I
 ;; think is a missed optimization on its end.
 ; LTO-DAG: (__TEXT,__const) non-external _local_unnamed_sometimes_const
 
 ;; The output here is largely identical to LD64's, except that the non-external
 ;; symbols here are all marked as (was a private external) by LD64. LLD should
 ;; follow suit.
 ; LTO-DYLIB-DAG: (__DATA,__data) non-external _global_unnamed
 ; LTO-DYLIB-DAG: (__DATA,__data) weak external _local_unnamed
 ; LTO-DYLIB-DAG: (__TEXT,__const) non-external _local_unnamed_always_const
 ; LTO-DYLIB-DAG: (__TEXT,__const) non-external _local_unnamed_const
 ; LTO-DYLIB-DAG: (__TEXT,__const) weak external _local_unnamed_sometimes_const
 
 ; THINLTO-DAG: (__DATA,__data) non-external (was a private external) _global_unnamed
 ; THINLTO-DAG: (__DATA,__data) weak external _local_unnamed
-;; The next two symbols are rendered as non-external (was a private external)
-;; by LD64. This is a missed optimization on LLD's end.
-; THINLTO-DAG: (__TEXT,__const) weak external _local_unnamed_always_const
-; THINLTO-DAG: (__TEXT,__const) weak external _local_unnamed_const
+; THINLTO-DAG: (__TEXT,__const) non-external (was a private external) _local_unnamed_always_const
+; THINLTO-DAG: (__TEXT,__const) non-external (was a private external) _local_unnamed_const
 ;; LD64 actually fails to link when the following symbol is included in the test
 ;; input, instead producing this error:
 ;; reference to bitcode symbol '_local_unnamed_sometimes_const' which LTO has not compiled in '_used' from /tmp/lto.o for architecture x86_64
 ; THINLTO-DAG: (__TEXT,__const) weak external _local_unnamed_sometimes_const
 
 ;--- test.ll
 target triple = "x86_64-apple-darwin"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

CallGraphEdges[Index.getOrInsertValueInfo(I)].updateHotness(
             : CalleeInfo::HotnessType::Critical);
 
   bool NonRenamableLocal = isNonRenamableLocal(F);
   bool NotEligibleForImport = NonRenamableLocal ||
                               HasInlineAsmMaybeReferencingInternal ||
                               HasIndirBranchToBlockAddress;
   GlobalValueSummary::GVFlags Flags(
       F.getLinkage(), F.getVisibility(), NotEligibleForImport,
       /* Live = */ false, F.isDSOLocal(),
-      F.hasLinkOnceODRLinkage() && F.hasGlobalUnnamedAddr());
+      F.canBeOmittedFromSymbolTable());
   FunctionSummary::FFlags FunFlags{
       F.hasFnAttribute(Attribute::ReadNone),
       F.hasFnAttribute(Attribute::ReadOnly),
       F.hasFnAttribute(Attribute::NoRecurse), F.returnDoesNotAlias(),
       // FIXME: refactor this to use the same code that inliner is using.
       // Don't try to import functions with noinline attribute.
       F.getAttributes().hasFnAttr(Attribute::NoInline),
       F.hasFnAttribute(Attribute::AlwaysInline),
static void computeVariableSummary(ModuleSummaryIndex &Index,
                                    const Module &M,
                                    SmallVectorImpl<MDNode *> &Types) {
   SetVector<ValueInfo> RefEdges;
   SmallPtrSet<const User *, 8> Visited;
   bool HasBlockAddress = findRefEdges(Index, &V, RefEdges, Visited);
   bool NonRenamableLocal = isNonRenamableLocal(V);
   GlobalValueSummary::GVFlags Flags(
       V.getLinkage(), V.getVisibility(), NonRenamableLocal,
       /* Live = */ false, V.isDSOLocal(),
-      V.hasLinkOnceODRLinkage() && V.hasGlobalUnnamedAddr());
+      V.canBeOmittedFromSymbolTable());
 
   VTableFuncList VTableFuncs;
   // If splitting is not enabled, then we compute the summary information
   // necessary for index-based whole program devirtualization.
   if (!Index.enableSplitLTOUnit()) {
     Types.clear();
     V.getMetadata(LLVMContext::MD_type, Types);
     if (!Types.empty()) {

 }
 
 static void
 computeAliasSummary(ModuleSummaryIndex &Index, const GlobalAlias &A,
                     DenseSet<GlobalValue::GUID> &CantBePromoted) {
   bool NonRenamableLocal = isNonRenamableLocal(A);
   GlobalValueSummary::GVFlags Flags(
       A.getLinkage(), A.getVisibility(), NonRenamableLocal,
       /* Live = */ false, A.isDSOLocal(),
-      A.hasLinkOnceODRLinkage() && A.hasGlobalUnnamedAddr());
+      A.canBeOmittedFromSymbolTable());
   auto AS = std::make_unique<AliasSummary>(Flags);
   auto *Aliasee = A.getAliaseeObject();
   auto AliaseeVI = Index.getValueInfo(Aliasee->getGUID());
   assert(AliaseeVI && "Alias expects aliasee summary to be available");
   assert(AliaseeVI.getSummaryList().size() == 1 &&
          "Expected a single entry per aliasee in per-module index");
   AS->setAliasee(AliaseeVI, AliaseeVI.getSummaryList()[0].get());
   if (NonRenamableLocal)
ModuleSymbolTable::CollectAsmSymbols(
           GlobalValue *GV = M.getNamedValue(Name);
           if (!GV)
             return;
           assert(GV->isDeclaration() && "Def in module asm already has definition");
           GlobalValueSummary::GVFlags GVFlags(
               GlobalValue::InternalLinkage, GlobalValue::DefaultVisibility,
               /* NotEligibleToImport = */ true,
               /* Live = */ true,
               /* Local */ GV->isDSOLocal(),
-              GV->hasLinkOnceODRLinkage() && GV->hasGlobalUnnamedAddr());
+              GV->canBeOmittedFromSymbolTable());
           CantBePromoted.insert(GV->getGUID());
           // Create the appropriate summary type.
           if (Function *F = dyn_cast<Function>(GV)) {
             std::unique_ptr<FunctionSummary> Summary =
                 std::make_unique<FunctionSummary>(
                     GVFlags, /*InstCount=*/0,
                     FunctionSummary::FFlags{
                         F->hasFnAttribute(Attribute::ReadNone),

llvm/lib/Transforms/IPO/FunctionImport.cpp

if (GlobalValue::isAvailableExternallyLinkage(NewLinkage) &&
         llvm_unreachable("Expected GV to be converted");
     } else {
       // If all copies of the original symbol had global unnamed addr and
       // linkonce_odr linkage, it should be an auto hide symbol. In that case
       // the thin link would have marked it as CanAutoHide. Add hidden visibility
       // to the symbol to preserve the property.
       if (NewLinkage == GlobalValue::WeakODRLinkage &&
           GS->second->canAutoHide()) {
-        assert(GV.hasLinkOnceODRLinkage() && GV.hasGlobalUnnamedAddr());
+        assert(GV.canBeOmittedFromSymbolTable());
         GV.setVisibility(GlobalValue::HiddenVisibility);
       }
 
       LLVM_DEBUG(dbgs() << "ODR fixing up linkage for `" << GV.getName()
                         << "` from " << GV.getLinkage() << " to " << NewLinkage
                         << "\n");
       GV.setLinkage(NewLinkage);
     }

llvm/test/ThinLTO/X86/Inputs/linkonce_odr_unnamed_addr.ll

 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-grtev4-linux-gnu"
 
-@linkonceodrunnamed = linkonce_odr unnamed_addr constant i32 0
+@linkonceodrunnamedconst = linkonce_odr unnamed_addr constant i32 0
+@linkonceodrunnamed = linkonce_odr unnamed_addr global i32 0
+@linkonceodrlocalunnamedconst = linkonce_odr local_unnamed_addr constant i32 0
+@linkonceodrlocalunnamed = linkonce_odr local_unnamed_addr global i32 0
 @odrunnamed = weak_odr unnamed_addr constant i32 0

llvm/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 when possible.
 
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/linkonce_odr_unnamed_addr.ll -o %t2.bc
 ; Check old LTO API
 ; 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 new LTO API
-; RUN: llvm-lto2 run -save-temps -o %t6.bc %t.bc %t2.bc -r=%t.bc,linkonceodrunnamed,p -r=%t.bc,odrunnamed,p -r=%t2.bc,linkonceodrunnamed, -r=%t2.bc,odrunnamed,
+; RUN: llvm-lto2 run -save-temps -o %t6.bc %t.bc %t2.bc \
+; RUN:   -r=%t.bc,linkonceodrunnamedconst,p \
+; RUN:   -r=%t.bc,linkonceodrunnamed,p \
+; RUN:   -r=%t.bc,linkonceodrlocalunnamedconst,p \
+; RUN:   -r=%t.bc,linkonceodrlocalunnamed,p \
+; RUN:   -r=%t.bc,odrunnamed,p \
+; RUN:   -r=%t2.bc,linkonceodrunnamedconst \
+; RUN:   -r=%t2.bc,linkonceodrunnamed \
+; RUN:   -r=%t2.bc,linkonceodrlocalunnamedconst \
+; RUN:   -r=%t2.bc,linkonceodrlocalunnamed \
+; RUN:   -r=%t2.bc,odrunnamed
 ; RUN: llvm-dis %t6.bc.1.1.promote.bc -o - | FileCheck %s
 
 ; Now test when one module does not have a summary. In that case we must be
 ; conservative and not auto hide.
 ; RUN: opt %p/Inputs/linkonce_odr_unnamed_addr.ll -o %t4.bc
 ; Check new LTO API (old LTO API does not detect this case).
-; RUN: llvm-lto2 run -save-temps -o %t6.bc %t.bc %t4.bc -r=%t.bc,linkonceodrunnamed,p -r=%t.bc,odrunnamed,p -r=%t4.bc,linkonceodrunnamed, -r=%t4.bc,odrunnamed,
+; RUN: llvm-lto2 run -save-temps -o %t6.bc %t.bc %t4.bc \
+; RUN:   -r=%t.bc,linkonceodrunnamedconst,p \
+; RUN:   -r=%t.bc,linkonceodrunnamed,p \
+; RUN:   -r=%t.bc,linkonceodrlocalunnamedconst,p \
+; RUN:   -r=%t.bc,linkonceodrlocalunnamed,p \
+; RUN:   -r=%t.bc,odrunnamed,p \
+; RUN:   -r=%t4.bc,linkonceodrunnamedconst \
+; RUN:   -r=%t4.bc,linkonceodrunnamed \
+; RUN:   -r=%t4.bc,linkonceodrlocalunnamedconst \
+; RUN:   -r=%t4.bc,linkonceodrlocalunnamed \
+; RUN:   -r=%t4.bc,odrunnamed
 ; RUN: llvm-dis %t6.bc.1.1.promote.bc -o - | FileCheck %s --check-prefix=NOSUMMARY
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-grtev4-linux-gnu"
 
 ; In this case all copies are linkonce_odr, so it may be hidden.
-; CHECK: @linkonceodrunnamed = weak_odr hidden unnamed_addr constant i32 0
-; NOSUMMARY: @linkonceodrunnamed = weak_odr unnamed_addr constant i32 0
-@linkonceodrunnamed = linkonce_odr unnamed_addr constant i32 0
+; CHECK: @linkonceodrunnamedconst = weak_odr hidden unnamed_addr constant i32 0
+; CHECK: @linkonceodrunnamed = weak_odr hidden unnamed_addr global i32 0
+; CHECK: @linkonceodrlocalunnamedconst = weak_odr hidden local_unnamed_addr constant i32 0
+; NOSUMMARY: @linkonceodrunnamedconst = weak_odr unnamed_addr constant i32 0
+; NOSUMMARY: @linkonceodrunnamed = weak_odr unnamed_addr global i32 0
+; NOSUMMARY: @linkonceodrlocalunnamedconst = weak_odr local_unnamed_addr constant i32 0
+@linkonceodrunnamedconst = linkonce_odr unnamed_addr constant i32 0
+@linkonceodrunnamed = linkonce_odr unnamed_addr global i32 0
+@linkonceodrlocalunnamedconst = linkonce_odr local_unnamed_addr constant i32 0
 
 ; In this case, the other copy was weak_odr, so it may not be hidden.
 ; CHECK: @odrunnamed = weak_odr unnamed_addr constant i32 0
 ; NOSUMMARY: @odrunnamed = weak_odr unnamed_addr constant i32 0
 @odrunnamed = linkonce_odr unnamed_addr constant i32 0
+
+; local_unnamed_addr globals cannot be hidden.
+; CHECK: @linkonceodrlocalunnamed = weak_odr local_unnamed_addr global i32 0
+; NOSUMMARY: @linkonceodrlocalunnamed = weak_odr local_unnamed_addr global i32 0
+@linkonceodrlocalunnamed = linkonce_odr local_unnamed_addr global i32 0