Skip to content

Commit

Permalink
[LTO] Initialize canAutoHide() using canBeOmittedFromSymbolTable()
Browse files Browse the repository at this point in the history
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.

Reviewed By: tejohnson

Differential Revision: https://reviews.llvm.org/D120173
  • Loading branch information
int3 committed Mar 4, 2022
1 parent 5c26874 commit dd29597
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 26 deletions.
10 changes: 4 additions & 6 deletions lld/test/MachO/lto-internalize-unnamed-addr.ll
Expand Up @@ -13,10 +13,10 @@
; 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
Expand All @@ -40,10 +40,8 @@

; 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
Expand Down
12 changes: 4 additions & 8 deletions llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
Expand Up @@ -489,8 +489,7 @@ static void computeFunctionSummary(
HasIndirBranchToBlockAddress;
GlobalValueSummary::GVFlags Flags(
F.getLinkage(), F.getVisibility(), NotEligibleForImport,
/* Live = */ false, F.isDSOLocal(),
F.hasLinkOnceODRLinkage() && F.hasGlobalUnnamedAddr());
/* Live = */ false, F.isDSOLocal(), F.canBeOmittedFromSymbolTable());
FunctionSummary::FFlags FunFlags{
F.hasFnAttribute(Attribute::ReadNone),
F.hasFnAttribute(Attribute::ReadOnly),
Expand Down Expand Up @@ -611,8 +610,7 @@ static void computeVariableSummary(ModuleSummaryIndex &Index,
bool NonRenamableLocal = isNonRenamableLocal(V);
GlobalValueSummary::GVFlags Flags(
V.getLinkage(), V.getVisibility(), NonRenamableLocal,
/* Live = */ false, V.isDSOLocal(),
V.hasLinkOnceODRLinkage() && V.hasGlobalUnnamedAddr());
/* Live = */ false, V.isDSOLocal(), V.canBeOmittedFromSymbolTable());

VTableFuncList VTableFuncs;
// If splitting is not enabled, then we compute the summary information
Expand Down Expand Up @@ -654,8 +652,7 @@ computeAliasSummary(ModuleSummaryIndex &Index, const GlobalAlias &A,
bool NonRenamableLocal = isNonRenamableLocal(A);
GlobalValueSummary::GVFlags Flags(
A.getLinkage(), A.getVisibility(), NonRenamableLocal,
/* Live = */ false, A.isDSOLocal(),
A.hasLinkOnceODRLinkage() && A.hasGlobalUnnamedAddr());
/* Live = */ false, A.isDSOLocal(), A.canBeOmittedFromSymbolTable());
auto AS = std::make_unique<AliasSummary>(Flags);
auto *Aliasee = A.getAliaseeObject();
auto AliaseeVI = Index.getValueInfo(Aliasee->getGUID());
Expand Down Expand Up @@ -732,8 +729,7 @@ ModuleSummaryIndex llvm::buildModuleSummaryIndex(
GlobalValue::InternalLinkage, GlobalValue::DefaultVisibility,
/* NotEligibleToImport = */ true,
/* Live = */ true,
/* Local */ GV->isDSOLocal(),
GV->hasLinkOnceODRLinkage() && GV->hasGlobalUnnamedAddr());
/* Local */ GV->isDSOLocal(), GV->canBeOmittedFromSymbolTable());
CantBePromoted.insert(GV->getGUID());
// Create the appropriate summary type.
if (Function *F = dyn_cast<Function>(GV)) {
Expand Down
9 changes: 5 additions & 4 deletions llvm/lib/Transforms/IPO/FunctionImport.cpp
Expand Up @@ -1112,12 +1112,13 @@ void llvm::thinLTOFinalizeInModule(Module &TheModule,
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.
// linkonce_odr linkage, or if all of them had local unnamed addr linkage
// and are constants, then 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);
}

Expand Down
5 changes: 4 additions & 1 deletion llvm/test/ThinLTO/X86/Inputs/linkonce_odr_unnamed_addr.ll
@@ -1,5 +1,8 @@
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
46 changes: 39 additions & 7 deletions llvm/test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll
@@ -1,31 +1,63 @@
; This test ensures that when linkonce_odr + unnamed_addr symbols promoted to
; weak symbols, it preserves the auto hide property when possible.
; This test ensures that when linkonce_odr + unnamed_addr symbols & linkonce_odr
; + local_unnamed_addr constants get promoted to weak symbols, we 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

0 comments on commit dd29597

Please sign in to comment.