[llvm] Revert "[ThinLTO] Populate declaration import status except for distributed ThinLTO under a default-off new option" (PR #92715)
via llvm-commits
llvm-commits at lists.llvm.org
Sun May 19 22:43:02 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lto
Author: Mingming Liu (minglotus-6)
<details>
<summary>Changes</summary>
Reverts 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:
- (modified) llvm/include/llvm/IR/ModuleSummaryIndex.h (-7)
- (modified) llvm/include/llvm/Transforms/IPO/FunctionImport.h (+5-10)
- (modified) llvm/lib/LTO/LTO.cpp (+12-20)
- (modified) llvm/lib/LTO/LTOBackend.cpp (+1-8)
- (modified) llvm/lib/Transforms/IPO/FunctionImport.cpp (+62-208)
- (modified) llvm/test/ThinLTO/X86/funcimport-stats.ll (+2-2)
- (removed) llvm/test/ThinLTO/X86/import_callee_declaration.ll (-180)
- (modified) llvm/test/Transforms/FunctionImport/funcimport.ll (+2-3)
- (modified) llvm/tools/llvm-link/llvm-link.cpp (+1-5)
``````````diff
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]
``````````
</details>
https://github.com/llvm/llvm-project/pull/92715
More information about the llvm-commits
mailing list