From 683e2bf059a6e5e0bb9dc2628218b53dc2d1b490 Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Tue, 21 Oct 2025 06:53:40 -0700 Subject: [PATCH] [ThinLTO] Make SummaryList private (NFC) (#164355) In preparation for a follow on change that will require checking every time a new summary is added to the SummaryList for a GUID, make the SummaryList private and require all accesses to go through one of two new interfaces. Most changes are to access the list via the read only getSummaryList() method, and the few that add new summaries (e.g. while building the combined summary) use the new addSummary() method. --- llvm/include/llvm/IR/ModuleSummaryIndex.h | 29 ++++++++++++++----- llvm/include/llvm/IR/ModuleSummaryIndexYAML.h | 8 ++--- llvm/lib/Analysis/ModuleSummaryAnalysis.cpp | 6 ++-- llvm/lib/Analysis/StackSafetyAnalysis.cpp | 4 +-- llvm/lib/Bitcode/Writer/BitcodeWriter.cpp | 4 +-- llvm/lib/IR/AsmWriter.cpp | 2 +- llvm/lib/IR/ModuleSummaryIndex.cpp | 12 ++++---- llvm/lib/LTO/LTO.cpp | 4 +-- llvm/lib/LTO/LTOBackend.cpp | 4 +-- llvm/lib/LTO/ThinLTOCodeGenerator.cpp | 15 +++++----- llvm/lib/Transforms/IPO/FunctionImport.cpp | 12 ++++---- llvm/lib/Transforms/IPO/LowerTypeTests.cpp | 4 +-- .../lib/Transforms/IPO/WholeProgramDevirt.cpp | 6 ++-- llvm/tools/llvm-link/llvm-link.cpp | 2 +- llvm/tools/llvm-lto/llvm-lto.cpp | 2 +- 15 files changed, 65 insertions(+), 49 deletions(-) diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h index ac79d91d417c..0062cece1794 100644 --- a/llvm/include/llvm/IR/ModuleSummaryIndex.h +++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h @@ -164,9 +164,24 @@ struct alignas(8) GlobalValueSummaryInfo { inline GlobalValueSummaryInfo(bool HaveGVs); + /// Access a read-only list of global value summary structures for a + /// particular value held in the GlobalValueMap. + ArrayRef> getSummaryList() const { + return SummaryList; + } + + /// Add a summary corresponding to a global value definition in a module with + /// the corresponding GUID. + void addSummary(std::unique_ptr Summary) { + return SummaryList.push_back(std::move(Summary)); + } + +private: /// List of global value summary structures for a particular value held /// in the GlobalValueMap. Requires a vector in the case of multiple - /// COMDAT values of the same name. + /// COMDAT values of the same name, weak symbols, locals of the same name when + /// compiling without sufficient distinguishing path, or (theoretically) hash + /// collisions. Each summary is from a different module. GlobalValueSummaryList SummaryList; }; @@ -201,7 +216,7 @@ struct ValueInfo { } ArrayRef> getSummaryList() const { - return getRef()->second.SummaryList; + return getRef()->second.getSummaryList(); } // Even if the index is built with GVs available, we may not have one for @@ -1607,8 +1622,8 @@ public: for (auto &S : *this) { // Skip external functions - if (!S.second.SummaryList.size() || - !isa(S.second.SummaryList.front().get())) + if (!S.second.getSummaryList().size() || + !isa(S.second.getSummaryList().front().get())) continue; discoverNodes(ValueInfo(HaveGVs, &S), FunctionHasParent); } @@ -1748,7 +1763,7 @@ public: // Here we have a notionally const VI, but the value it points to is owned // by the non-const *this. const_cast(VI.getRef()) - ->second.SummaryList.push_back(std::move(Summary)); + ->second.addSummary(std::move(Summary)); } /// Add an original name for the value of the given GUID. @@ -1937,7 +1952,7 @@ public: collectDefinedGVSummariesPerModule(Map &ModuleToDefinedGVSummaries) const { for (const auto &GlobalList : *this) { auto GUID = GlobalList.first; - for (const auto &Summary : GlobalList.second.SummaryList) { + for (const auto &Summary : GlobalList.second.getSummaryList()) { ModuleToDefinedGVSummaries[Summary->modulePath()][GUID] = Summary.get(); } } @@ -2035,7 +2050,7 @@ struct GraphTraits : public GraphTraits { std::unique_ptr Root = std::make_unique(I->calculateCallGraphRoot()); GlobalValueSummaryInfo G(I->haveGVs()); - G.SummaryList.push_back(std::move(Root)); + G.addSummary(std::move(Root)); static auto P = GlobalValueSummaryMapTy::value_type(GlobalValue::GUID(0), std::move(G)); return ValueInfo(I->haveGVs(), &P); diff --git a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h index 531de514822e..3381e1777217 100644 --- a/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h +++ b/llvm/include/llvm/IR/ModuleSummaryIndexYAML.h @@ -237,7 +237,7 @@ template <> struct CustomMappingTraits { // This is done in fixAliaseeLinks() which is called in // MappingTraits::mapping(). ASum->setAliasee(AliaseeVI, /*Aliasee=*/nullptr); - Elem.SummaryList.push_back(std::move(ASum)); + Elem.addSummary(std::move(ASum)); continue; } SmallVector Refs; @@ -246,7 +246,7 @@ template <> struct CustomMappingTraits { auto It = V.try_emplace(RefGUID, /*IsAnalysis=*/false).first; Refs.push_back(ValueInfo(/*IsAnalysis=*/false, &*It)); } - Elem.SummaryList.push_back(std::make_unique( + Elem.addSummary(std::make_unique( GVFlags, /*NumInsts=*/0, FunctionSummary::FFlags{}, std::move(Refs), SmallVector{}, std::move(GVSum.TypeTests), std::move(GVSum.TypeTestAssumeVCalls), @@ -260,7 +260,7 @@ template <> struct CustomMappingTraits { static void output(IO &io, GlobalValueSummaryMapTy &V) { for (auto &P : V) { std::vector GVSums; - for (auto &Sum : P.second.SummaryList) { + for (auto &Sum : P.second.getSummaryList()) { if (auto *FSum = dyn_cast(Sum.get())) { std::vector Refs; Refs.reserve(FSum->refs().size()); @@ -295,7 +295,7 @@ template <> struct CustomMappingTraits { } static void fixAliaseeLinks(GlobalValueSummaryMapTy &V) { for (auto &P : V) { - for (auto &Sum : P.second.SummaryList) { + for (auto &Sum : P.second.getSummaryList()) { if (auto *Alias = dyn_cast(Sum.get())) { ValueInfo AliaseeVI = Alias->getAliaseeVI(); auto AliaseeSL = AliaseeVI.getSummaryList(); diff --git a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp index a60a4bb1194e..6cdf51a148e2 100644 --- a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp +++ b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp @@ -1100,12 +1100,12 @@ ModuleSummaryIndex llvm::buildModuleSummaryIndex( for (auto &GlobalList : Index) { // Ignore entries for references that are undefined in the current module. - if (GlobalList.second.SummaryList.empty()) + if (GlobalList.second.getSummaryList().empty()) continue; - assert(GlobalList.second.SummaryList.size() == 1 && + assert(GlobalList.second.getSummaryList().size() == 1 && "Expected module's index to have one summary per GUID"); - auto &Summary = GlobalList.second.SummaryList[0]; + auto &Summary = GlobalList.second.getSummaryList()[0]; if (!IsThinLTO) { Summary->setNotEligibleToImport(); continue; diff --git a/llvm/lib/Analysis/StackSafetyAnalysis.cpp b/llvm/lib/Analysis/StackSafetyAnalysis.cpp index 5e94e0bfe673..5e92ca1d38e7 100644 --- a/llvm/lib/Analysis/StackSafetyAnalysis.cpp +++ b/llvm/lib/Analysis/StackSafetyAnalysis.cpp @@ -1136,7 +1136,7 @@ void llvm::generateParamAccessSummary(ModuleSummaryIndex &Index) { if (!AreStatisticsEnabled()) return; for (auto &GVS : Index) - for (auto &GV : GVS.second.SummaryList) + for (auto &GV : GVS.second.getSummaryList()) if (FunctionSummary *FS = dyn_cast(GV.get())) Stat += FS->paramAccesses().size(); }; @@ -1147,7 +1147,7 @@ void llvm::generateParamAccessSummary(ModuleSummaryIndex &Index) { // Convert the ModuleSummaryIndex to a FunctionMap for (auto &GVS : Index) { - for (auto &GV : GVS.second.SummaryList) { + for (auto &GV : GVS.second.getSummaryList()) { FunctionSummary *FS = dyn_cast(GV.get()); if (!FS || FS->paramAccesses().empty()) continue; diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp index 8ff3aa981757..61aa7c2f5af5 100644 --- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp +++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp @@ -235,7 +235,7 @@ public: return; for (const auto &GUIDSummaryLists : *Index) // Examine all summaries for this GUID. - for (auto &Summary : GUIDSummaryLists.second.SummaryList) + for (auto &Summary : GUIDSummaryLists.second.getSummaryList()) if (auto FS = dyn_cast(Summary.get())) { // For each call in the function summary, see if the call // is to a GUID (which means it is for an indirect call, @@ -587,7 +587,7 @@ public: } } else { for (auto &Summaries : Index) - for (auto &Summary : Summaries.second.SummaryList) + for (auto &Summary : Summaries.second.getSummaryList()) Callback({Summaries.first, Summary.get()}, false); } } diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp index 3908a78f4841..488b078ab6ca 100644 --- a/llvm/lib/IR/AsmWriter.cpp +++ b/llvm/lib/IR/AsmWriter.cpp @@ -3196,7 +3196,7 @@ void AssemblyWriter::printModuleSummaryIndex() { // for aliasee (then update BitcodeWriter.cpp and remove get/setAliaseeGUID). for (auto &GlobalList : *TheIndex) { auto GUID = GlobalList.first; - for (auto &Summary : GlobalList.second.SummaryList) + for (auto &Summary : GlobalList.second.getSummaryList()) SummaryToGUIDMap[Summary.get()] = GUID; } diff --git a/llvm/lib/IR/ModuleSummaryIndex.cpp b/llvm/lib/IR/ModuleSummaryIndex.cpp index dc55b63c8b79..a6353664e0e7 100644 --- a/llvm/lib/IR/ModuleSummaryIndex.cpp +++ b/llvm/lib/IR/ModuleSummaryIndex.cpp @@ -162,7 +162,7 @@ void ModuleSummaryIndex::collectDefinedFunctionsForModule( StringRef ModulePath, GVSummaryMapTy &GVSummaryMap) const { for (auto &GlobalList : *this) { auto GUID = GlobalList.first; - for (auto &GlobSummary : GlobalList.second.SummaryList) { + for (auto &GlobSummary : GlobalList.second.getSummaryList()) { auto *Summary = dyn_cast_or_null(GlobSummary.get()); if (!Summary) // Ignore global variable, focus on functions @@ -263,7 +263,7 @@ void ModuleSummaryIndex::propagateAttributes( DenseSet MarkedNonReadWriteOnly; for (auto &P : *this) { bool IsDSOLocal = true; - for (auto &S : P.second.SummaryList) { + for (auto &S : P.second.getSummaryList()) { if (!isGlobalValueLive(S.get())) { // computeDeadSymbolsAndUpdateIndirectCalls should have marked all // copies live. Note that it is possible that there is a GUID collision @@ -273,7 +273,7 @@ void ModuleSummaryIndex::propagateAttributes( // all copies live we can assert here that all are dead if any copy is // dead. assert(llvm::none_of( - P.second.SummaryList, + P.second.getSummaryList(), [&](const std::unique_ptr &Summary) { return isGlobalValueLive(Summary.get()); })); @@ -308,16 +308,16 @@ void ModuleSummaryIndex::propagateAttributes( // Mark the flag in all summaries false so that we can do quick check // without going through the whole list. for (const std::unique_ptr &Summary : - P.second.SummaryList) + P.second.getSummaryList()) Summary->setDSOLocal(false); } setWithAttributePropagation(); setWithDSOLocalPropagation(); if (llvm::AreStatisticsEnabled()) for (auto &P : *this) - if (P.second.SummaryList.size()) + if (P.second.getSummaryList().size()) if (auto *GVS = dyn_cast( - P.second.SummaryList[0]->getBaseObject())) + P.second.getSummaryList()[0]->getBaseObject())) if (isGlobalValueLive(GVS)) { if (GVS->maybeReadOnly()) ReadOnlyLiveGVars++; diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp index aec8891eabe7..cbc0b1d3256b 100644 --- a/llvm/lib/LTO/LTO.cpp +++ b/llvm/lib/LTO/LTO.cpp @@ -457,7 +457,7 @@ void llvm::thinLTOResolvePrevailingInIndex( // when needed. DenseSet GlobalInvolvedWithAlias; for (auto &I : Index) - for (auto &S : I.second.SummaryList) + for (auto &S : I.second.getSummaryList()) if (auto AS = dyn_cast(S.get())) GlobalInvolvedWithAlias.insert(&AS->getAliasee()); @@ -1182,7 +1182,7 @@ Error LTO::checkPartiallySplit() { // Otherwise check if there are any recorded in the combined summary from the // ThinLTO modules. for (auto &P : ThinLTO.CombinedIndex) { - for (auto &S : P.second.SummaryList) { + for (auto &S : P.second.getSummaryList()) { auto *FS = dyn_cast(S.get()); if (!FS) continue; diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp index 280c3d1d8dfa..93118becedba 100644 --- a/llvm/lib/LTO/LTOBackend.cpp +++ b/llvm/lib/LTO/LTOBackend.cpp @@ -770,11 +770,11 @@ bool lto::initImportList(const Module &M, // via a WriteIndexesThinBackend. for (const auto &GlobalList : CombinedIndex) { // Ignore entries for undefined references. - if (GlobalList.second.SummaryList.empty()) + if (GlobalList.second.getSummaryList().empty()) continue; auto GUID = GlobalList.first; - for (const auto &Summary : GlobalList.second.SummaryList) { + for (const auto &Summary : GlobalList.second.getSummaryList()) { // Skip the summaries for the importing module. These are included to // e.g. record required linkage changes. if (Summary->modulePath() == M.getModuleIdentifier()) diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp index 5b333cda31fb..ff94c54ab3e6 100644 --- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp +++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp @@ -101,8 +101,8 @@ static void saveTempBitcode(const Module &TheModule, StringRef TempDir, WriteBitcodeToFile(TheModule, OS, /* ShouldPreserveUseListOrder */ true); } -static const GlobalValueSummary * -getFirstDefinitionForLinker(const GlobalValueSummaryList &GVSummaryList) { +static const GlobalValueSummary *getFirstDefinitionForLinker( + ArrayRef> GVSummaryList) { // If there is any strong definition anywhere, get it. auto StrongDefForLinker = llvm::find_if( GVSummaryList, [](const std::unique_ptr &Summary) { @@ -131,14 +131,15 @@ getFirstDefinitionForLinker(const GlobalValueSummaryList &GVSummaryList) { static void computePrevailingCopies( const ModuleSummaryIndex &Index, DenseMap &PrevailingCopy) { - auto HasMultipleCopies = [&](const GlobalValueSummaryList &GVSummaryList) { - return GVSummaryList.size() > 1; - }; + auto HasMultipleCopies = + [&](ArrayRef> GVSummaryList) { + return GVSummaryList.size() > 1; + }; for (auto &I : Index) { - if (HasMultipleCopies(I.second.SummaryList)) + if (HasMultipleCopies(I.second.getSummaryList())) PrevailingCopy[I.first] = - getFirstDefinitionForLinker(I.second.SummaryList); + getFirstDefinitionForLinker(I.second.getSummaryList()); } } diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp index 28ee4449421b..a29faab6d007 100644 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -1368,13 +1368,13 @@ static void ComputeCrossModuleImportForModuleFromIndexForTest( FunctionImporter::ImportMapTy &ImportList) { for (const auto &GlobalList : Index) { // Ignore entries for undefined references. - if (GlobalList.second.SummaryList.empty()) + if (GlobalList.second.getSummaryList().empty()) continue; auto GUID = GlobalList.first; - assert(GlobalList.second.SummaryList.size() == 1 && + assert(GlobalList.second.getSummaryList().size() == 1 && "Expected individual combined index to have one summary per GUID"); - auto &Summary = GlobalList.second.SummaryList[0]; + auto &Summary = GlobalList.second.getSummaryList()[0]; // Skip the summaries for the importing module. These are included to // e.g. record required linkage changes. if (Summary->modulePath() == ModulePath) @@ -1423,7 +1423,7 @@ void updateValueInfoForIndirectCalls(ModuleSummaryIndex &Index, void llvm::updateIndirectCalls(ModuleSummaryIndex &Index) { for (const auto &Entry : Index) { - for (const auto &S : Entry.second.SummaryList) { + for (const auto &S : Entry.second.getSummaryList()) { if (auto *FS = dyn_cast(S.get())) updateValueInfoForIndirectCalls(Index, FS); } @@ -1456,7 +1456,7 @@ void llvm::computeDeadSymbolsAndUpdateIndirectCalls( // Add values flagged in the index as live roots to the worklist. for (const auto &Entry : Index) { auto VI = Index.getValueInfo(Entry); - for (const auto &S : Entry.second.SummaryList) { + for (const auto &S : Entry.second.getSummaryList()) { if (auto *FS = dyn_cast(S.get())) updateValueInfoForIndirectCalls(Index, FS); if (S->isLive()) { @@ -2094,7 +2094,7 @@ static bool doImportingForModuleForTest( // is only enabled when testing importing via the 'opt' tool, which does // not do the ThinLink that would normally determine what values to promote. for (auto &I : *Index) { - for (auto &S : I.second.SummaryList) { + for (auto &S : I.second.getSummaryList()) { if (GlobalValue::isLocalLinkage(S->linkage())) S->setLinkage(GlobalValue::ExternalLinkage); } diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp index be6cba38d8b3..46fb567593ad 100644 --- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp +++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp @@ -2130,7 +2130,7 @@ bool LowerTypeTestsModule::lower() { // A set of all functions that are address taken by a live global object. DenseSet AddressTaken; for (auto &I : *ExportSummary) - for (auto &GVS : I.second.SummaryList) + for (auto &GVS : I.second.getSummaryList()) if (GVS->isLive()) for (const auto &Ref : GVS->refs()) { AddressTaken.insert(Ref.getGUID()); @@ -2409,7 +2409,7 @@ bool LowerTypeTestsModule::lower() { } for (auto &P : *ExportSummary) { - for (auto &S : P.second.SummaryList) { + for (auto &S : P.second.getSummaryList()) { if (!ExportSummary->isGlobalValueLive(S.get())) continue; if (auto *FS = dyn_cast(S->getBaseObject())) diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp index 2d5cb8268ffd..76e588b116c0 100644 --- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp +++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp @@ -928,7 +928,7 @@ void llvm::updateVCallVisibilityInIndex( // linker, as we have no information on their eventual use. if (DynamicExportSymbols.count(P.first)) continue; - for (auto &S : P.second.SummaryList) { + for (auto &S : P.second.getSummaryList()) { auto *GVar = dyn_cast(S.get()); if (!GVar || GVar->getVCallVisibility() != GlobalObject::VCallVisibilityPublic) @@ -2413,7 +2413,7 @@ bool DevirtModule::run() { } for (auto &P : *ExportSummary) { - for (auto &S : P.second.SummaryList) { + for (auto &S : P.second.getSummaryList()) { auto *FS = dyn_cast(S.get()); if (!FS) continue; @@ -2564,7 +2564,7 @@ void DevirtIndex::run() { // Collect information from summary about which calls to try to devirtualize. for (auto &P : ExportSummary) { - for (auto &S : P.second.SummaryList) { + for (auto &S : P.second.getSummaryList()) { auto *FS = dyn_cast(S.get()); if (!FS) continue; diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp index 93b1fb6031ce..33c3e6fc350f 100644 --- a/llvm/tools/llvm-link/llvm-link.cpp +++ b/llvm/tools/llvm-link/llvm-link.cpp @@ -420,7 +420,7 @@ static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L, // does not do the ThinLink that would normally determine what values to // promote. for (auto &I : *Index) { - for (auto &S : I.second.SummaryList) { + for (auto &S : I.second.getSummaryList()) { if (GlobalValue::isLocalLinkage(S->linkage())) S->setLinkage(GlobalValue::ExternalLinkage); } diff --git a/llvm/tools/llvm-lto/llvm-lto.cpp b/llvm/tools/llvm-lto/llvm-lto.cpp index 05e9502e3abb..09f7142d0dcd 100644 --- a/llvm/tools/llvm-lto/llvm-lto.cpp +++ b/llvm/tools/llvm-lto/llvm-lto.cpp @@ -379,7 +379,7 @@ static void printIndexStats() { unsigned Calls = 0, Refs = 0, Functions = 0, Alias = 0, Globals = 0; for (auto &Summaries : *Index) { - for (auto &Summary : Summaries.second.SummaryList) { + for (auto &Summary : Summaries.second.getSummaryList()) { Refs += Summary->refs().size(); if (auto *FuncSummary = dyn_cast(Summary.get())) { Functions++;