From dd29597e103cd13dd308fb68283fce9d3411f723 Mon Sep 17 00:00:00 2001 From: Jez Ng Date: Thu, 3 Mar 2022 19:04:11 -0500 Subject: [PATCH] [LTO] Initialize canAutoHide() using canBeOmittedFromSymbolTable() 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 --- lld/test/MachO/lto-internalize-unnamed-addr.ll | 10 ++--- llvm/lib/Analysis/ModuleSummaryAnalysis.cpp | 12 ++---- llvm/lib/Transforms/IPO/FunctionImport.cpp | 9 +++-- .../X86/Inputs/linkonce_odr_unnamed_addr.ll | 5 ++- llvm/test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll | 46 ++++++++++++++++++---- 5 files changed, 56 insertions(+), 26 deletions(-) diff --git a/lld/test/MachO/lto-internalize-unnamed-addr.ll b/lld/test/MachO/lto-internalize-unnamed-addr.ll index e0622d6..2b4f982 100644 --- a/lld/test/MachO/lto-internalize-unnamed-addr.ll +++ b/lld/test/MachO/lto-internalize-unnamed-addr.ll @@ -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 @@ -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 diff --git a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp index 63c4aa3..8bf70c5 100644 --- a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp +++ b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp @@ -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), @@ -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 @@ -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(Flags); auto *Aliasee = A.getAliaseeObject(); auto AliaseeVI = Index.getValueInfo(Aliasee->getGUID()); @@ -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(GV)) { diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp index d9b4310..7f90c2c 100644 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -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); } diff --git a/llvm/test/ThinLTO/X86/Inputs/linkonce_odr_unnamed_addr.ll b/llvm/test/ThinLTO/X86/Inputs/linkonce_odr_unnamed_addr.ll index e0bb068..99c0312 100644 --- a/llvm/test/ThinLTO/X86/Inputs/linkonce_odr_unnamed_addr.ll +++ b/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 diff --git a/llvm/test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll b/llvm/test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll index 8b8e367..46bafa0 100644 --- a/llvm/test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll +++ b/llvm/test/ThinLTO/X86/linkonce_odr_unnamed_addr.ll @@ -1,5 +1,6 @@ -; 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 @@ -7,25 +8,56 @@ ; 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 -- 2.7.4