[ThinLTO] Auto-hide prevailing linkonce_odr only when all copies eligible
Summary:
We hit undefined references building with ThinLTO when one source file
contained explicit instantiations of a template method (weak_odr) but
there were also implicit instantiations in another file (linkonce_odr),
and the latter was the prevailing copy. In this case the symbol was
marked hidden when the prevailing linkonce_odr copy was promoted to
weak_odr. It led to unsats when the resulting shared library was linked
with other code that contained a reference (expecting to be resolved due
to the explicit instantiation).
Add a CanAutoHide flag to the GV summary to allow the thin link to
identify when all copies are eligible for auto-hiding (because they were
all originally linkonce_odr global unnamed addr), and only do the
auto-hide in that case.
Most of the changes here are due to plumbing the new flag through the
bitcode and llvm assembly, and resulting test changes. I augmented the
existing auto-hide test to check for this situation.
Reviewers: pcc
Subscribers: mehdi_amini, inglorion, eraman, dexonsmith, arphaman, dang, llvm-commits, steven_wu, wmi
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D59709
llvm-svn: 360466
diff --git a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
index 68eb1ef9..5282c29 100644
--- a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
+++ b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
@@ -391,7 +391,8 @@
bool NotEligibleForImport =
NonRenamableLocal || HasInlineAsmMaybeReferencingInternal;
GlobalValueSummary::GVFlags Flags(F.getLinkage(), NotEligibleForImport,
- /* Live = */ false, F.isDSOLocal());
+ /* Live = */ false, F.isDSOLocal(),
+ F.hasLinkOnceODRLinkage() && F.hasGlobalUnnamedAddr());
FunctionSummary::FFlags FunFlags{
F.hasFnAttribute(Attribute::ReadNone),
F.hasFnAttribute(Attribute::ReadOnly),
@@ -418,7 +419,8 @@
bool HasBlockAddress = findRefEdges(Index, &V, RefEdges, Visited);
bool NonRenamableLocal = isNonRenamableLocal(V);
GlobalValueSummary::GVFlags Flags(V.getLinkage(), NonRenamableLocal,
- /* Live = */ false, V.isDSOLocal());
+ /* Live = */ false, V.isDSOLocal(),
+ V.hasLinkOnceODRLinkage() && V.hasGlobalUnnamedAddr());
// Don't mark variables we won't be able to internalize as read-only.
GlobalVarSummary::GVarFlags VarFlags(
@@ -438,7 +440,8 @@
DenseSet<GlobalValue::GUID> &CantBePromoted) {
bool NonRenamableLocal = isNonRenamableLocal(A);
GlobalValueSummary::GVFlags Flags(A.getLinkage(), NonRenamableLocal,
- /* Live = */ false, A.isDSOLocal());
+ /* Live = */ false, A.isDSOLocal(),
+ A.hasLinkOnceODRLinkage() && A.hasGlobalUnnamedAddr());
auto AS = llvm::make_unique<AliasSummary>(Flags);
auto *Aliasee = A.getBaseObject();
auto AliaseeVI = Index.getValueInfo(Aliasee->getGUID());
@@ -513,7 +516,8 @@
GlobalValueSummary::GVFlags GVFlags(GlobalValue::InternalLinkage,
/* NotEligibleToImport = */ true,
/* Live = */ true,
- /* Local */ GV->isDSOLocal());
+ /* Local */ GV->isDSOLocal(),
+ GV->hasLinkOnceODRLinkage() && GV->hasGlobalUnnamedAddr());
CantBePromoted.insert(GV->getGUID());
// Create the appropriate summary type.
if (Function *F = dyn_cast<Function>(GV)) {
diff --git a/llvm/lib/AsmParser/LLLexer.cpp b/llvm/lib/AsmParser/LLLexer.cpp
index bc3776d..c0b9cd1 100644
--- a/llvm/lib/AsmParser/LLLexer.cpp
+++ b/llvm/lib/AsmParser/LLLexer.cpp
@@ -733,6 +733,7 @@
KEYWORD(notEligibleToImport);
KEYWORD(live);
KEYWORD(dsoLocal);
+ KEYWORD(canAutoHide);
KEYWORD(function);
KEYWORD(insts);
KEYWORD(funcFlags);
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 907bbdd..9558bd0 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -7859,7 +7859,7 @@
StringRef ModulePath;
GlobalValueSummary::GVFlags GVFlags = GlobalValueSummary::GVFlags(
/*Linkage=*/GlobalValue::ExternalLinkage, /*NotEligibleToImport=*/false,
- /*Live=*/false, /*IsLocal=*/false);
+ /*Live=*/false, /*IsLocal=*/false, /*CanAutoHide=*/false);
unsigned InstCount;
std::vector<FunctionSummary::EdgeTy> Calls;
FunctionSummary::TypeIdInfo TypeIdInfo;
@@ -7929,7 +7929,7 @@
StringRef ModulePath;
GlobalValueSummary::GVFlags GVFlags = GlobalValueSummary::GVFlags(
/*Linkage=*/GlobalValue::ExternalLinkage, /*NotEligibleToImport=*/false,
- /*Live=*/false, /*IsLocal=*/false);
+ /*Live=*/false, /*IsLocal=*/false, /*CanAutoHide=*/false);
GlobalVarSummary::GVarFlags GVarFlags(/*ReadOnly*/ false);
std::vector<ValueInfo> Refs;
if (ParseToken(lltok::colon, "expected ':' here") ||
@@ -7972,7 +7972,7 @@
StringRef ModulePath;
GlobalValueSummary::GVFlags GVFlags = GlobalValueSummary::GVFlags(
/*Linkage=*/GlobalValue::ExternalLinkage, /*NotEligibleToImport=*/false,
- /*Live=*/false, /*IsLocal=*/false);
+ /*Live=*/false, /*IsLocal=*/false, /*CanAutoHide=*/false);
if (ParseToken(lltok::colon, "expected ':' here") ||
ParseToken(lltok::lparen, "expected '(' here") ||
ParseModuleReference(ModulePath) ||
@@ -8462,41 +8462,55 @@
/// GVFlags
/// ::= 'flags' ':' '(' 'linkage' ':' OptionalLinkageAux ','
/// 'notEligibleToImport' ':' Flag ',' 'live' ':' Flag ','
-/// 'dsoLocal' ':' Flag ')'
+/// 'dsoLocal' ':' Flag ',' 'canAutoHide' ':' Flag ')'
bool LLParser::ParseGVFlags(GlobalValueSummary::GVFlags &GVFlags) {
assert(Lex.getKind() == lltok::kw_flags);
Lex.Lex();
- bool HasLinkage;
if (ParseToken(lltok::colon, "expected ':' here") ||
- ParseToken(lltok::lparen, "expected '(' here") ||
- ParseToken(lltok::kw_linkage, "expected 'linkage' here") ||
- ParseToken(lltok::colon, "expected ':' here"))
+ ParseToken(lltok::lparen, "expected '(' here"))
return true;
- GVFlags.Linkage = parseOptionalLinkageAux(Lex.getKind(), HasLinkage);
- assert(HasLinkage && "Linkage not optional in summary entry");
- Lex.Lex();
-
- unsigned Flag;
- if (ParseToken(lltok::comma, "expected ',' here") ||
- ParseToken(lltok::kw_notEligibleToImport,
- "expected 'notEligibleToImport' here") ||
- ParseToken(lltok::colon, "expected ':' here") || ParseFlag(Flag))
- return true;
- GVFlags.NotEligibleToImport = Flag;
-
- if (ParseToken(lltok::comma, "expected ',' here") ||
- ParseToken(lltok::kw_live, "expected 'live' here") ||
- ParseToken(lltok::colon, "expected ':' here") || ParseFlag(Flag))
- return true;
- GVFlags.Live = Flag;
-
- if (ParseToken(lltok::comma, "expected ',' here") ||
- ParseToken(lltok::kw_dsoLocal, "expected 'dsoLocal' here") ||
- ParseToken(lltok::colon, "expected ':' here") || ParseFlag(Flag))
- return true;
- GVFlags.DSOLocal = Flag;
+ do {
+ unsigned Flag;
+ switch (Lex.getKind()) {
+ case lltok::kw_linkage:
+ Lex.Lex();
+ if (ParseToken(lltok::colon, "expected ':'"))
+ return true;
+ bool HasLinkage;
+ GVFlags.Linkage = parseOptionalLinkageAux(Lex.getKind(), HasLinkage);
+ assert(HasLinkage && "Linkage not optional in summary entry");
+ Lex.Lex();
+ break;
+ case lltok::kw_notEligibleToImport:
+ Lex.Lex();
+ if (ParseToken(lltok::colon, "expected ':'") || ParseFlag(Flag))
+ return true;
+ GVFlags.NotEligibleToImport = Flag;
+ break;
+ case lltok::kw_live:
+ Lex.Lex();
+ if (ParseToken(lltok::colon, "expected ':'") || ParseFlag(Flag))
+ return true;
+ GVFlags.Live = Flag;
+ break;
+ case lltok::kw_dsoLocal:
+ Lex.Lex();
+ if (ParseToken(lltok::colon, "expected ':'") || ParseFlag(Flag))
+ return true;
+ GVFlags.DSOLocal = Flag;
+ break;
+ case lltok::kw_canAutoHide:
+ Lex.Lex();
+ if (ParseToken(lltok::colon, "expected ':'") || ParseFlag(Flag))
+ return true;
+ GVFlags.CanAutoHide = Flag;
+ break;
+ default:
+ return Error(Lex.getLoc(), "expected gv flag type");
+ }
+ } while (EatIfPresent(lltok::comma));
if (ParseToken(lltok::rparen, "expected ')' here"))
return true;
diff --git a/llvm/lib/AsmParser/LLToken.h b/llvm/lib/AsmParser/LLToken.h
index 80a1eb9..33ea28b 100644
--- a/llvm/lib/AsmParser/LLToken.h
+++ b/llvm/lib/AsmParser/LLToken.h
@@ -364,6 +364,7 @@
kw_notEligibleToImport,
kw_live,
kw_dsoLocal,
+ kw_canAutoHide,
kw_function,
kw_insts,
kw_funcFlags,
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 5eff6e6..d3b8c4d 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -893,8 +893,9 @@
// values as live.
bool Live = (RawFlags & 0x2) || Version < 3;
bool Local = (RawFlags & 0x4);
+ bool AutoHide = (RawFlags & 0x8);
- return GlobalValueSummary::GVFlags(Linkage, NotEligibleToImport, Live, Local);
+ return GlobalValueSummary::GVFlags(Linkage, NotEligibleToImport, Live, Local, AutoHide);
}
// Decode the flags for GlobalVariable in the summary
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index c835976..63b0aab 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -996,6 +996,7 @@
RawFlags |= Flags.NotEligibleToImport; // bool
RawFlags |= (Flags.Live << 1);
RawFlags |= (Flags.DSOLocal << 2);
+ RawFlags |= (Flags.CanAutoHide << 3);
// 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
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 376262b..b5db8bd 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -3041,6 +3041,7 @@
Out << ", notEligibleToImport: " << GVFlags.NotEligibleToImport;
Out << ", live: " << GVFlags.Live;
Out << ", dsoLocal: " << GVFlags.DSOLocal;
+ Out << ", canAutoHide: " << GVFlags.CanAutoHide;
Out << ")";
if (Summary.getSummaryKind() == GlobalValueSummary::AliasKind)
diff --git a/llvm/lib/IR/ModuleSummaryIndex.cpp b/llvm/lib/IR/ModuleSummaryIndex.cpp
index 4704215..18b7ac0 100644
--- a/llvm/lib/IR/ModuleSummaryIndex.cpp
+++ b/llvm/lib/IR/ModuleSummaryIndex.cpp
@@ -26,6 +26,7 @@
FunctionSummary FunctionSummary::ExternalNode =
FunctionSummary::makeDummyFunctionSummary({});
+
bool ValueInfo::isDSOLocal() const {
// Need to check all summaries are local in case of hash collisions.
return getSummaryList().size() &&
@@ -35,6 +36,15 @@
});
}
+bool ValueInfo::canAutoHide() const {
+ // Can only auto hide if all copies are eligible to auto hide.
+ return getSummaryList().size() &&
+ llvm::all_of(getSummaryList(),
+ [](const std::unique_ptr<GlobalValueSummary> &Summary) {
+ return Summary->canAutoHide();
+ });
+}
+
// Gets the number of immutable refs in RefEdgeList
unsigned FunctionSummary::immutableRefCount() const {
// Here we take advantage of having all readonly references
@@ -399,6 +409,10 @@
if (Flags.Live && hasReadOnlyFlag(SummaryIt.second))
A.addComment("immutable");
}
+ if (Flags.DSOLocal)
+ A.addComment("dsoLocal");
+ if (Flags.CanAutoHide)
+ A.addComment("canAutoHide");
auto VI = getValueInfo(SummaryIt.first);
A.add("label", getNodeLabel(VI, SummaryIt.second));
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index ab5b780..6a3bf1a 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -187,6 +187,7 @@
auto AddUsedThings = [&](GlobalValueSummary *GS) {
if (!GS) return;
AddUnsigned(GS->isLive());
+ AddUnsigned(GS->canAutoHide());
for (const ValueInfo &VI : GS->refs()) {
AddUnsigned(VI.isDSOLocal());
AddUsedCfiGlobal(VI.getGUID());
@@ -295,13 +296,13 @@
}
static void thinLTOResolvePrevailingGUID(
- GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
- DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
+ ValueInfo VI, DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing,
function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
- recordNewLinkage) {
- for (auto &S : GVSummaryList) {
+ recordNewLinkage,
+ const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols) {
+ for (auto &S : VI.getSummaryList()) {
GlobalValue::LinkageTypes OriginalLinkage = S->linkage();
// Ignore local and appending linkage values since the linker
// doesn't resolve them.
@@ -316,17 +317,29 @@
// ensure a copy is kept to satisfy the exported reference.
// FIXME: We may want to split the compile time and correctness
// aspects into separate routines.
- if (isPrevailing(GUID, S.get())) {
- if (GlobalValue::isLinkOnceLinkage(OriginalLinkage))
+ if (isPrevailing(VI.getGUID(), S.get())) {
+ if (GlobalValue::isLinkOnceLinkage(OriginalLinkage)) {
S->setLinkage(GlobalValue::getWeakLinkage(
GlobalValue::isLinkOnceODRLinkage(OriginalLinkage)));
+ // The kept copy is eligible for auto-hiding (hidden visibility) if all
+ // copies were (i.e. they were all linkonce_odr global unnamed addr).
+ // If any copy is not (e.g. it was originally weak_odr), then the symbol
+ // must remain externally available (e.g. a weak_odr from an explicitly
+ // instantiated template). Additionally, if it is in the
+ // GUIDPreservedSymbols set, that means that it is visibile outside
+ // the summary (e.g. in a native object or a bitcode file without
+ // summary), and in that case we cannot hide it as it isn't possible to
+ // check all copies.
+ S->setCanAutoHide(VI.canAutoHide() &&
+ !GUIDPreservedSymbols.count(VI.getGUID()));
+ }
}
// Alias and aliasee can't be turned into available_externally.
else if (!isa<AliasSummary>(S.get()) &&
!GlobalInvolvedWithAlias.count(S.get()))
S->setLinkage(GlobalValue::AvailableExternallyLinkage);
if (S->linkage() != OriginalLinkage)
- recordNewLinkage(S->modulePath(), GUID, S->linkage());
+ recordNewLinkage(S->modulePath(), VI.getGUID(), S->linkage());
}
}
@@ -341,7 +354,8 @@
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing,
function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
- recordNewLinkage) {
+ recordNewLinkage,
+ const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols) {
// 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.
@@ -352,9 +366,9 @@
GlobalInvolvedWithAlias.insert(&AS->getAliasee());
for (auto &I : Index)
- thinLTOResolvePrevailingGUID(I.second.SummaryList, I.first,
- GlobalInvolvedWithAlias, isPrevailing,
- recordNewLinkage);
+ thinLTOResolvePrevailingGUID(Index.getValueInfo(I), GlobalInvolvedWithAlias,
+ isPrevailing, recordNewLinkage,
+ GUIDPreservedSymbols);
}
static void thinLTOInternalizeAndPromoteGUID(
@@ -903,7 +917,7 @@
Error Result = runRegularLTO(AddStream);
if (!Result)
- Result = runThinLTO(AddStream, Cache);
+ Result = runThinLTO(AddStream, Cache, GUIDPreservedSymbols);
if (StatsFile)
PrintStatisticsJSON(StatsFile->os());
@@ -1206,7 +1220,8 @@
};
}
-Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache) {
+Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache,
+ const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols) {
if (ThinLTO.ModuleMap.empty())
return Error::success();
@@ -1288,7 +1303,7 @@
ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
};
thinLTOResolvePrevailingInIndex(ThinLTO.CombinedIndex, isPrevailing,
- recordNewLinkage);
+ recordNewLinkage, GUIDPreservedSymbols);
std::unique_ptr<ThinBackendProc> BackendProc =
ThinLTO.Backend(Conf, ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index 43b20fb..df18dd3 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -457,7 +457,8 @@
static void resolvePrevailingInIndex(
ModuleSummaryIndex &Index,
StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>>
- &ResolvedODR) {
+ &ResolvedODR,
+ const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols) {
DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
computePrevailingCopies(Index, PrevailingCopy);
@@ -476,7 +477,8 @@
ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
};
- thinLTOResolvePrevailingInIndex(Index, isPrevailing, recordNewLinkage);
+ thinLTOResolvePrevailingInIndex(Index, isPrevailing, recordNewLinkage,
+ GUIDPreservedSymbols);
}
// Initialize the TargetMachine builder for a given Triple
@@ -630,7 +632,7 @@
// Resolve prevailing symbols
StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
- resolvePrevailingInIndex(Index, ResolvedODR);
+ resolvePrevailingInIndex(Index, ResolvedODR, GUIDPreservedSymbols);
thinLTOResolvePrevailingInModule(
TheModule, ModuleToDefinedGVSummaries[ModuleIdentifier]);
@@ -786,7 +788,7 @@
// Resolve prevailing symbols
StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
- resolvePrevailingInIndex(Index, ResolvedODR);
+ resolvePrevailingInIndex(Index, ResolvedODR, GUIDPreservedSymbols);
// Promote the exported values in the index, so that they are promoted
// in the module.
@@ -945,7 +947,7 @@
// Resolve prevailing symbols, this has to be computed early because it
// impacts the caching.
- resolvePrevailingInIndex(*Index, ResolvedODR);
+ resolvePrevailingInIndex(*Index, ResolvedODR, GUIDPreservedSymbols);
// Use global summary-based analysis to identify symbols that can be
// internalized (because they aren't exported or preserved as per callback).
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index ef8b152..71a76a6 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -976,12 +976,15 @@
// 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 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());
GV.setVisibility(GlobalValue::HiddenVisibility);
+ }
LLVM_DEBUG(dbgs() << "ODR fixing up linkage for `" << GV.getName()
<< "` from " << GV.getLinkage() << " to " << NewLinkage