diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h --- a/llvm/include/llvm/IR/ModuleSummaryIndex.h +++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h @@ -700,6 +700,8 @@ /// Return the list of pairs. ArrayRef calls() const { return CallGraphEdgeList; } + std::vector &mutableCalls() { return CallGraphEdgeList; } + void addCall(EdgeTy E) { CallGraphEdgeList.push_back(E); } /// Returns the list of type identifiers used by this function in diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h --- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h +++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h @@ -167,16 +167,24 @@ FunctionImporter::ImportMapTy &ImportList); /// PrevailingType enum used as a return type of callback passed -/// to computeDeadSymbols. Yes and No values used when status explicitly -/// set by symbols resolution, otherwise status is Unknown. +/// to computeDeadSymbolsAndUpdateIndirectCalls. Yes and No values used when +/// status explicitly set by symbols resolution, otherwise status is Unknown. enum class PrevailingType { Yes, No, Unknown }; +/// Update call edges for indirect calls to local functions added from +/// SamplePGO when needed. Normally this is done during +/// computeDeadSymbolsAndUpdateIndirectCalls, but can be called standalone +/// when that is not called (e.g. during testing). +void updateIndirectCalls(ModuleSummaryIndex &Index); + /// Compute all the symbols that are "dead": i.e these that can't be reached /// in the graph from any of the given symbols listed in /// \p GUIDPreservedSymbols. Non-prevailing symbols are symbols without a /// prevailing copy anywhere in IR and are normally dead, \p isPrevailing /// predicate returns status of symbol. -void computeDeadSymbols( +/// Also update call edges for indirect calls to local functions added from +/// SamplePGO when needed. +void computeDeadSymbolsAndUpdateIndirectCalls( ModuleSummaryIndex &Index, const DenseSet &GUIDPreservedSymbols, function_ref isPrevailing); diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp --- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp +++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp @@ -4208,33 +4208,7 @@ } auto GetValueId = [&](const ValueInfo &VI) -> Optional { - GlobalValue::GUID GUID = VI.getGUID(); - Optional CallValueId = getValueId(GUID); - if (CallValueId) - return CallValueId; - // For SamplePGO, the indirect call targets for local functions will - // have its original name annotated in profile. We try to find the - // corresponding PGOFuncName as the GUID. - GUID = Index.getGUIDFromOriginalID(GUID); - if (!GUID) - return None; - CallValueId = getValueId(GUID); - if (!CallValueId) - return None; - // The mapping from OriginalId to GUID may return a GUID - // that corresponds to a static variable. Filter it out here. - // This can happen when - // 1) There is a call to a library function which does not have - // a CallValidId; - // 2) There is a static variable with the OriginalGUID identical - // to the GUID of the library function in 1); - // When this happens, the logic for SamplePGO kicks in and - // the static variable in 2) will be found, which needs to be - // filtered out. - auto *GVSum = Index.getGlobalValueSummary(GUID, false); - if (GVSum && GVSum->getSummaryKind() == GlobalValueSummary::GlobalVarKind) - return None; - return CallValueId; + return getValueId(VI.getGUID()); }; auto *FS = cast(S); diff --git a/llvm/lib/IR/ModuleSummaryIndex.cpp b/llvm/lib/IR/ModuleSummaryIndex.cpp --- a/llvm/lib/IR/ModuleSummaryIndex.cpp +++ b/llvm/lib/IR/ModuleSummaryIndex.cpp @@ -251,12 +251,13 @@ bool IsDSOLocal = true; for (auto &S : P.second.SummaryList) { if (!isGlobalValueLive(S.get())) { - // computeDeadSymbols should have marked all copies live. Note that - // it is possible that there is a GUID collision between internal - // symbols with the same name in different files of the same name but - // not enough distinguishing path. Because computeDeadSymbols should - // conservatively mark all copies live we can assert here that all are - // dead if any copy is dead. + // computeDeadSymbolsAndUpdateIndirectCalls should have marked all + // copies live. Note that it is possible that there is a GUID collision + // between internal symbols with the same name in different files of the + // same name but not enough distinguishing path. Because + // computeDeadSymbolsAndUpdateIndirectCalls should conservatively mark + // all copies live we can assert here that all are dead if any copy is + // dead. assert(llvm::none_of( P.second.SummaryList, [&](const std::unique_ptr &Summary) { diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -188,23 +188,6 @@ return false; } - // For SamplePGO, in computeImportForFunction the OriginalId - // may have been used to locate the callee summary list (See - // comment there). - // The mapping from OriginalId to GUID may return a GUID - // that corresponds to a static variable. Filter it out here. - // This can happen when - // 1) There is a call to a library function which is not defined - // in the index. - // 2) There is a static variable with the OriginalGUID identical - // to the GUID of the library function in 1); - // When this happens, the logic for SamplePGO kicks in and - // the static variable in 2) will be found, which needs to be - // filtered out. - if (GVSummary->getSummaryKind() == GlobalValueSummary::GlobalVarKind) { - Reason = FunctionImporter::ImportFailureReason::GlobalVar; - return false; - } if (GlobalValue::isInterposableLinkage(GVSummary->linkage())) { Reason = FunctionImporter::ImportFailureReason::InterposableLinkage; // There is no point in importing these, we can't inline them @@ -265,21 +248,6 @@ } // anonymous namespace -static ValueInfo -updateValueInfoForIndirectCalls(const ModuleSummaryIndex &Index, ValueInfo VI) { - if (!VI.getSummaryList().empty()) - return VI; - // For SamplePGO, the indirect call targets for local functions will - // have its original name annotated in profile. We try to find the - // corresponding PGOFuncName as the GUID. - // FIXME: Consider updating the edges in the graph after building - // it, rather than needing to perform this mapping on each walk. - auto GUID = Index.getGUIDFromOriginalID(VI.getGUID()); - if (GUID == 0) - return ValueInfo(); - return Index.getValueInfo(GUID); -} - static bool shouldImportGlobal(const ValueInfo &VI, const GVSummaryMapTy &DefinedGVSummaries) { const auto &GVS = DefinedGVSummaries.find(VI.getGUID()); @@ -401,10 +369,6 @@ continue; } - VI = updateValueInfoForIndirectCalls(Index, VI); - if (!VI) - continue; - if (DefinedGVSummaries.count(VI.getGUID())) { // FIXME: Consider not skipping import if the module contains // a non-prevailing def with interposable linkage. The prevailing copy @@ -840,16 +804,61 @@ #endif } -void llvm::computeDeadSymbols( +// For SamplePGO, the indirect call targets for local functions will +// have its original name annotated in profile. We try to find the +// corresponding PGOFuncName as the GUID, and fix up the edges +// accordingly. +void updateValueInfoForIndirectCalls(ModuleSummaryIndex &Index, + FunctionSummary *FS) { + for (auto &EI : FS->mutableCalls()) { + if (!EI.first.getSummaryList().empty()) + continue; + auto GUID = Index.getGUIDFromOriginalID(EI.first.getGUID()); + if (GUID == 0) + continue; + // Update the edge to point directly to the correct GUID. + auto VI = Index.getValueInfo(GUID); + if (llvm::any_of( + VI.getSummaryList(), + [&](const std::unique_ptr &SummaryPtr) { + // The mapping from OriginalId to GUID may return a GUID + // that corresponds to a static variable. Filter it out here. + // This can happen when + // 1) There is a call to a library function which is not defined + // in the index. + // 2) There is a static variable with the OriginalGUID identical + // to the GUID of the library function in 1); + // When this happens the static variable in 2) will be found, + // which needs to be filtered out. + return SummaryPtr->getSummaryKind() == + GlobalValueSummary::GlobalVarKind; + })) + continue; + EI.first = VI; + } +} + +void llvm::updateIndirectCalls(ModuleSummaryIndex &Index) { + for (const auto &Entry : Index) { + for (auto &S : Entry.second.SummaryList) { + if (auto *FS = dyn_cast(S.get())) + updateValueInfoForIndirectCalls(Index, FS); + } + } +} + +void llvm::computeDeadSymbolsAndUpdateIndirectCalls( ModuleSummaryIndex &Index, const DenseSet &GUIDPreservedSymbols, function_ref isPrevailing) { assert(!Index.withGlobalValueDeadStripping()); - if (!ComputeDead) - return; - if (GUIDPreservedSymbols.empty()) - // Don't do anything when nothing is live, this is friendly with tests. + if (!ComputeDead || + // Don't do anything when nothing is live, this is friendly with tests. + GUIDPreservedSymbols.empty()) { + // Still need to update indirect calls. + updateIndirectCalls(Index); return; + } unsigned LiveSymbols = 0; SmallVector Worklist; Worklist.reserve(GUIDPreservedSymbols.size() * 2); @@ -864,13 +873,16 @@ // Add values flagged in the index as live roots to the worklist. for (const auto &Entry : Index) { auto VI = Index.getValueInfo(Entry); - for (auto &S : Entry.second.SummaryList) + for (auto &S : Entry.second.SummaryList) { + if (auto *FS = dyn_cast(S.get())) + updateValueInfoForIndirectCalls(Index, FS); if (S->isLive()) { LLVM_DEBUG(dbgs() << "Live root: " << VI << "\n"); Worklist.push_back(VI); ++LiveSymbols; break; } + } } // Make value live and add it to the worklist if it was not live before. @@ -883,9 +895,6 @@ // binary, which increases the binary size unnecessarily. Note that // if this code changes, the importer needs to change so that edges // to functions marked dead are skipped. - VI = updateValueInfoForIndirectCalls(Index, VI); - if (!VI) - return; if (llvm::any_of(VI.getSummaryList(), [](const std::unique_ptr &S) { @@ -959,7 +968,8 @@ const DenseSet &GUIDPreservedSymbols, function_ref isPrevailing, bool ImportEnabled) { - computeDeadSymbols(Index, GUIDPreservedSymbols, isPrevailing); + computeDeadSymbolsAndUpdateIndirectCalls(Index, GUIDPreservedSymbols, + isPrevailing); if (ImportEnabled) Index.propagateAttributes(GUIDPreservedSymbols); } diff --git a/llvm/tools/llvm-lto/llvm-lto.cpp b/llvm/tools/llvm-lto/llvm-lto.cpp --- a/llvm/tools/llvm-lto/llvm-lto.cpp +++ b/llvm/tools/llvm-lto/llvm-lto.cpp @@ -487,6 +487,10 @@ ExitOnErr(errorOrToExpected(MemoryBuffer::getFileOrSTDIN(Filename))); ExitOnErr(readModuleSummaryIndex(*MB, CombinedIndex, NextModuleId++)); } + // In order to use this index for testing, specifically import testing, we + // need to update any indirect call edges created from SamplePGO, so that they + // point to the correct GUIDs. + updateIndirectCalls(CombinedIndex); std::error_code EC; assert(!OutputFilename.empty()); raw_fd_ostream OS(OutputFilename + ".thinlto.bc", EC,