Revert "[ThinLTO] Populate declaration import status except for distributed ThinLTO under a default-off new option" #92715
+85
−443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ibuted T…" This reverts commit 8de7890.
llvmbot
added
LTO
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-lto Author: Mingming Liu (minglotus-6) ChangesReverts llvm/llvm-project#88024 Build bot failures (https://lab.llvm.org/buildbot/#/builders/259/builds/4727 and https://lab.llvm.org/buildbot/#/builders/9/builds/43876) Patch is 41.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92715.diff 9 Files Affected:
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index a6bb261af7522..5d137d4b3553c 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -587,10 +587,6 @@ class GlobalValueSummary {
void setImportKind(ImportKind IK) { Flags.ImportType = IK; }
- GlobalValueSummary::ImportKind importType() const {
- return static_cast<ImportKind>(Flags.ImportType);
- }
-
GlobalValue::VisibilityTypes getVisibility() const {
return (GlobalValue::VisibilityTypes)Flags.Visibility;
}
@@ -1276,9 +1272,6 @@ using ModulePathStringTableTy = StringMap<ModuleHash>;
/// a particular module, and provide efficient access to their summary.
using GVSummaryMapTy = DenseMap<GlobalValue::GUID, GlobalValueSummary *>;
-/// A set of global value summary pointers.
-using GVSummaryPtrSet = SmallPtrSet<GlobalValueSummary *, 4>;
-
/// Map of a type GUID to type id string and summary (multimap used
/// in case of GUID conflicts).
using TypeIdSummaryMapTy =
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index 024bba8105b89..c4d19e8641eca 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -31,9 +31,9 @@ class Module;
/// based on the provided summary informations.
class FunctionImporter {
public:
- /// The functions to import from a source module and their import type.
- using FunctionsToImportTy =
- DenseMap<GlobalValue::GUID, GlobalValueSummary::ImportKind>;
+ /// Set of functions to import from a source module. Each entry is a set
+ /// containing all the GUIDs of all functions to import for a source module.
+ using FunctionsToImportTy = std::unordered_set<GlobalValue::GUID>;
/// The different reasons selectCallee will chose not to import a
/// candidate.
@@ -99,13 +99,8 @@ class FunctionImporter {
/// index's module path string table).
using ImportMapTy = DenseMap<StringRef, FunctionsToImportTy>;
- /// The map contains an entry for every global value the module exports.
- /// The key is ValueInfo, and the value indicates whether the definition
- /// or declaration is visible to another module. If a function's definition is
- /// visible to other modules, the global values this function referenced are
- /// visible and shouldn't be internalized.
- /// TODO: Rename to `ExportMapTy`.
- using ExportSetTy = DenseMap<ValueInfo, GlobalValueSummary::ImportKind>;
+ /// The set contains an entry for every global value the module exports.
+ using ExportSetTy = DenseSet<ValueInfo>;
/// A function of this type is used to load modules referenced by the index.
using ModuleLoaderTy =
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index e2754d74979e8..5c603ac6ab472 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -121,9 +121,6 @@ void llvm::computeLTOCacheKey(
support::endian::write64le(Data, I);
Hasher.update(Data);
};
- auto AddUint8 = [&](const uint8_t I) {
- Hasher.update(ArrayRef<uint8_t>((const uint8_t *)&I, 1));
- };
AddString(Conf.CPU);
// FIXME: Hash more of Options. For now all clients initialize Options from
// command-line flags (which is unsupported in production), but may set
@@ -159,18 +156,18 @@ void llvm::computeLTOCacheKey(
auto ModHash = Index.getModuleHash(ModuleID);
Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
- std::vector<std::pair<uint64_t, uint8_t>> ExportsGUID;
+ std::vector<uint64_t> ExportsGUID;
ExportsGUID.reserve(ExportList.size());
- for (const auto &[VI, ExportType] : ExportList)
- ExportsGUID.push_back(
- std::make_pair(VI.getGUID(), static_cast<uint8_t>(ExportType)));
+ for (const auto &VI : ExportList) {
+ auto GUID = VI.getGUID();
+ ExportsGUID.push_back(GUID);
+ }
// Sort the export list elements GUIDs.
llvm::sort(ExportsGUID);
- for (auto [GUID, ExportType] : ExportsGUID) {
+ for (uint64_t GUID : ExportsGUID) {
// The export list can impact the internalization, be conservative here
Hasher.update(ArrayRef<uint8_t>((uint8_t *)&GUID, sizeof(GUID)));
- AddUint8(ExportType);
}
// Include the hash for every module we import functions from. The set of
@@ -202,7 +199,7 @@ void llvm::computeLTOCacheKey(
[](const ImportModule &Lhs, const ImportModule &Rhs) -> bool {
return Lhs.getHash() < Rhs.getHash();
});
- std::vector<std::pair<uint64_t, uint8_t>> ImportedGUIDs;
+ std::vector<uint64_t> ImportedGUIDs;
for (const ImportModule &Entry : ImportModulesVector) {
auto ModHash = Entry.getHash();
Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
@@ -210,13 +207,11 @@ void llvm::computeLTOCacheKey(
AddUint64(Entry.getFunctions().size());
ImportedGUIDs.clear();
- for (auto &[Fn, ImportType] : Entry.getFunctions())
- ImportedGUIDs.push_back(std::make_pair(Fn, ImportType));
+ for (auto &Fn : Entry.getFunctions())
+ ImportedGUIDs.push_back(Fn);
llvm::sort(ImportedGUIDs);
- for (auto &[GUID, Type] : ImportedGUIDs) {
+ for (auto &GUID : ImportedGUIDs)
AddUint64(GUID);
- AddUint8(Type);
- }
}
// Include the hash for the resolved ODR.
@@ -286,9 +281,9 @@ void llvm::computeLTOCacheKey(
// Imported functions may introduce new uses of type identifier resolutions,
// so we need to collect their used resolutions as well.
for (const ImportModule &ImpM : ImportModulesVector)
- for (auto &[GUID, UnusedImportType] : ImpM.getFunctions()) {
+ for (auto &ImpF : ImpM.getFunctions()) {
GlobalValueSummary *S =
- Index.findSummaryInModule(GUID, ImpM.getIdentifier());
+ Index.findSummaryInModule(ImpF, ImpM.getIdentifier());
AddUsedThings(S);
// If this is an alias, we also care about any types/etc. that the aliasee
// may reference.
@@ -1400,7 +1395,6 @@ class lto::ThinBackendProc {
llvm::StringRef ModulePath,
const std::string &NewModulePath) {
std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
-
std::error_code EC;
gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
ImportList, ModuleToSummariesForIndex);
@@ -1409,8 +1403,6 @@ class lto::ThinBackendProc {
sys::fs::OpenFlags::OF_None);
if (EC)
return errorCodeToError(EC);
-
- // TODO: Serialize declaration bits to bitcode.
writeIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex);
if (ShouldEmitImportsFiles) {
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index 58434feec6f96..d4b89ede2d713 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -721,14 +721,7 @@ bool lto::initImportList(const Module &M,
if (Summary->modulePath() == M.getModuleIdentifier())
continue;
// Add an entry to provoke importing by thinBackend.
- // Try emplace the entry first. If an entry with the same key already
- // exists, set the value to 'std::min(existing-value, new-value)' to make
- // sure a definition takes precedence over a declaration.
- auto [Iter, Inserted] = ImportList[Summary->modulePath()].try_emplace(
- GUID, Summary->importType());
-
- if (!Inserted)
- Iter->second = std::min(Iter->second, Summary->importType());
+ ImportList[Summary->modulePath()].insert(GUID);
}
}
return true;
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index a116fd6535347..68f9799616ae6 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -140,17 +140,6 @@ static cl::opt<bool>
ImportAllIndex("import-all-index",
cl::desc("Import all external functions in index."));
-/// This is a test-only option.
-/// If this option is enabled, the ThinLTO indexing step will import each
-/// function declaration as a fallback. In a real build this may increase ram
-/// usage of the indexing step unnecessarily.
-/// TODO: Implement selective import (based on combined summary analysis) to
-/// ensure the imported function has a use case in the postlink pipeline.
-static cl::opt<bool> ImportDeclaration(
- "import-declaration", cl::init(false), cl::Hidden,
- cl::desc("If true, import function declaration as fallback if the function "
- "definition is not imported."));
-
/// Pass a workload description file - an example of workload would be the
/// functions executed to satisfy a RPC request. A workload is defined by a root
/// function and the list of functions that are (frequently) needed to satisfy
@@ -256,12 +245,8 @@ static auto qualifyCalleeCandidates(
}
/// Given a list of possible callee implementation for a call site, select one
-/// that fits the \p Threshold for function definition import. If none are
-/// found, the Reason will give the last reason for the failure (last, in the
-/// order of CalleeSummaryList entries). While looking for a callee definition,
-/// sets \p TooLargeOrNoInlineSummary to the last seen too-large or noinline
-/// candidate; other modules may want to know the function summary or
-/// declaration even if a definition is not needed.
+/// that fits the \p Threshold. If none are found, the Reason will give the last
+/// reason for the failure (last, in the order of CalleeSummaryList entries).
///
/// FIXME: select "best" instead of first that fits. But what is "best"?
/// - The smallest: more likely to be inlined.
@@ -274,32 +259,24 @@ static const GlobalValueSummary *
selectCallee(const ModuleSummaryIndex &Index,
ArrayRef<std::unique_ptr<GlobalValueSummary>> CalleeSummaryList,
unsigned Threshold, StringRef CallerModulePath,
- const GlobalValueSummary *&TooLargeOrNoInlineSummary,
FunctionImporter::ImportFailureReason &Reason) {
- // Records the last summary with reason noinline or too-large.
- TooLargeOrNoInlineSummary = nullptr;
auto QualifiedCandidates =
qualifyCalleeCandidates(Index, CalleeSummaryList, CallerModulePath);
for (auto QualifiedValue : QualifiedCandidates) {
Reason = QualifiedValue.first;
- // Skip a summary if its import is not (proved to be) legal.
if (Reason != FunctionImporter::ImportFailureReason::None)
continue;
auto *Summary =
cast<FunctionSummary>(QualifiedValue.second->getBaseObject());
- // Don't bother importing the definition if the chance of inlining it is
- // not high enough (except under `--force-import-all`).
if ((Summary->instCount() > Threshold) && !Summary->fflags().AlwaysInline &&
!ForceImportAll) {
- TooLargeOrNoInlineSummary = Summary;
Reason = FunctionImporter::ImportFailureReason::TooLarge;
continue;
}
- // Don't bother importing the definition if we can't inline it anyway.
+ // Don't bother importing if we can't inline it anyway.
if (Summary->fflags().NoInline && !ForceImportAll) {
- TooLargeOrNoInlineSummary = Summary;
Reason = FunctionImporter::ImportFailureReason::NoInline;
continue;
}
@@ -381,27 +358,17 @@ class GlobalsImporter final {
if (!GVS || !Index.canImportGlobalVar(GVS, /* AnalyzeRefs */ true) ||
LocalNotInModule(GVS))
continue;
-
- // If there isn't an entry for GUID, insert <GUID, Definition> pair.
- // Otherwise, definition should take precedence over declaration.
- auto [Iter, Inserted] =
- ImportList[RefSummary->modulePath()].try_emplace(
- VI.getGUID(), GlobalValueSummary::Definition);
+ auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
// Only update stat and exports if we haven't already imported this
// variable.
- if (!Inserted) {
- // Set the value to 'std::min(existing-value, new-value)' to make
- // sure a definition takes precedence over a declaration.
- Iter->second = std::min(GlobalValueSummary::Definition, Iter->second);
+ if (!ILI.second)
break;
- }
NumImportedGlobalVarsThinLink++;
// Any references made by this variable will be marked exported
// later, in ComputeCrossModuleImport, after import decisions are
// complete, which is more efficient than adding them here.
if (ExportLists)
- (*ExportLists)[RefSummary->modulePath()][VI] =
- GlobalValueSummary::Definition;
+ (*ExportLists)[RefSummary->modulePath()].insert(VI);
// If variable is not writeonly we attempt to recursively analyze
// its references in order to import referenced constants.
@@ -578,11 +545,10 @@ class WorkloadImportsManager : public ModuleImportsManager {
LLVM_DEBUG(dbgs() << "[Workload][Including]" << VI.name() << " from "
<< ExportingModule << " : "
<< Function::getGUID(VI.name()) << "\n");
- ImportList[ExportingModule][VI.getGUID()] =
- GlobalValueSummary::Definition;
+ ImportList[ExportingModule].insert(VI.getGUID());
GVI.onImportingSummary(*GVS);
if (ExportLists)
- (*ExportLists)[ExportingModule][VI] = GlobalValueSummary::Definition;
+ (*ExportLists)[ExportingModule].insert(VI);
}
LLVM_DEBUG(dbgs() << "[Workload] Done\n");
}
@@ -803,28 +769,9 @@ static void computeImportForFunction(
}
FunctionImporter::ImportFailureReason Reason{};
-
- // `SummaryForDeclImport` is an summary eligible for declaration import.
- const GlobalValueSummary *SummaryForDeclImport = nullptr;
- CalleeSummary =
- selectCallee(Index, VI.getSummaryList(), NewThreshold,
- Summary.modulePath(), SummaryForDeclImport, Reason);
+ CalleeSummary = selectCallee(Index, VI.getSummaryList(), NewThreshold,
+ Summary.modulePath(), Reason);
if (!CalleeSummary) {
- // There isn't a callee for definition import but one for declaration
- // import.
- if (ImportDeclaration && SummaryForDeclImport) {
- StringRef DeclSourceModule = SummaryForDeclImport->modulePath();
-
- // Since definition takes precedence over declaration for the same VI,
- // try emplace <VI, declaration> pair without checking insert result.
- // If insert doesn't happen, there must be an existing entry keyed by
- // VI.
- if (ExportLists)
- (*ExportLists)[DeclSourceModule].try_emplace(
- VI, GlobalValueSummary::Declaration);
- ImportList[DeclSourceModule].try_emplace(
- VI.getGUID(), GlobalValueSummary::Declaration);
- }
// Update with new larger threshold if this was a retry (otherwise
// we would have already inserted with NewThreshold above). Also
// update failure info if requested.
@@ -869,15 +816,11 @@ static void computeImportForFunction(
"selectCallee() didn't honor the threshold");
auto ExportModulePath = ResolvedCalleeSummary->modulePath();
-
- // Try emplace the definition entry, and update stats based on insertion
- // status.
- auto [Iter, Inserted] = ImportList[ExportModulePath].try_emplace(
- VI.getGUID(), GlobalValueSummary::Definition);
-
+ auto ILI = ImportList[ExportModulePath].insert(VI.getGUID());
// We previously decided to import this GUID definition if it was already
// inserted in the set of imports from the exporting module.
- if (Inserted || Iter->second == GlobalValueSummary::Declaration) {
+ bool PreviouslyImported = !ILI.second;
+ if (!PreviouslyImported) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
@@ -885,14 +828,11 @@ static void computeImportForFunction(
NumImportedCriticalFunctionsThinLink++;
}
- if (Iter->second == GlobalValueSummary::Declaration)
- Iter->second = GlobalValueSummary::Definition;
-
// Any calls/references made by this function will be marked exported
// later, in ComputeCrossModuleImport, after import decisions are
// complete, which is more efficient than adding them here.
if (ExportLists)
- (*ExportLists)[ExportModulePath][VI] = GlobalValueSummary::Definition;
+ (*ExportLists)[ExportModulePath].insert(VI);
}
auto GetAdjustedThreshold = [](unsigned Threshold, bool IsHotCallsite) {
@@ -999,20 +939,12 @@ static bool isGlobalVarSummary(const ModuleSummaryIndex &Index,
}
template <class T>
-static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, T &Cont,
- unsigned &DefinedGVS,
- unsigned &DefinedFS) {
+static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index,
+ T &Cont) {
unsigned NumGVS = 0;
- DefinedGVS = 0;
- DefinedFS = 0;
- for (auto &[GUID, Type] : Cont) {
- if (isGlobalVarSummary(Index, GUID)) {
- if (Type == GlobalValueSummary::Definition)
- ++DefinedGVS;
+ for (auto &V : Cont)
+ if (isGlobalVarSummary(Index, V))
++NumGVS;
- } else if (Type == GlobalValueSummary::Definition)
- ++DefinedFS;
- }
return NumGVS;
}
#endif
@@ -1022,12 +954,13 @@ static bool checkVariableImport(
const ModuleSummaryIndex &Index,
DenseMap<StringRef, FunctionImporter::ImportMapTy> &ImportLists,
DenseMap<StringRef, FunctionImporter::ExportSetTy> &ExportLists) {
+
DenseSet<GlobalValue::GUID> FlattenedImports;
for (auto &ImportPerModule : ImportLists)
for (auto &ExportPerModule : ImportPerModule.second)
- for (auto &[GUID, Type] : ExportPerModule.second)
- FlattenedImports.insert(GUID);
+ FlattenedImports.insert(ExportPerModule.second.begin(),
+ ExportPerModule.second.end());
// Checks that all GUIDs of read/writeonly vars we see in export lists
// are also in the import lists. Otherwise we my face linker undefs,
@@ -1046,7 +979,7 @@ static bool checkVariableImport(
};
for (auto &ExportPerModule : ExportLists)
- for (auto &[VI, Unused] : ExportPerModule.second)
+ for (auto &VI : ExportPerModule.second)
if (!FlattenedImports.count(VI.getGUID()) &&
IsReadOrWriteOnlyVarNeedingImporting(ExportPerModule.first, VI))
return false;
@@ -1082,11 +1015,7 @@ void llvm::ComputeCrossModuleImport(
FunctionImporter::ExportSetTy NewExports;
const auto &DefinedGVSummaries =
ModuleToDefinedGVSummaries.lookup(ELI.first);
- for (auto &[EI, Type] : ELI.second) {
- // If a variable is exported as a declaration, its 'refs' and 'calls' are
- // not further exported.
- if (Type == GlobalValueSummary::Declaration)
- continue;
+ for (auto &EI : ELI.second) {
// Find the copy defined in the exporting module so that we can mark the
// values it references in that specific definition as exported.
// Below we will add all references and called values, without regard to
@@ -1105,31 +1034,22 @@ void llvm::ComputeCrossModuleImport(
// we convert such variables initializers to "zeroinitializer".
// See processGlobalForThinLTO.
if (!Index.isWriteOnly(GVS))
- for (const auto &VI : GVS->refs()) {
- // Try to emplace the declaration entry. If a definition entry
- // already exists for key `VI`, this is a no-op.
- NewExports.try_emplace(VI, GlobalValueSummary::Declaration);
- }
+ for (const auto &VI : GVS->refs())
+ NewExports.insert(VI);
} else {
auto *FS = cast<FunctionSummary>(S);
- for (const auto &Edge : FS->calls()) {
- // Try to emplace the declaration entry. If a definition entry
- // already exists for key `VI`, this is a no-op.
- NewExports.try_emplace(Edge.first, GlobalValueSummary::Declaration);
- }
- for (const auto &Ref : FS->refs()) {
...
[truncated]
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #88024
Build bot failures (https://lab.llvm.org/buildbot/#/builders/259/builds/4727 and https://lab.llvm.org/buildbot/#/builders/9/builds/43876)