[ThinLTO] Populate declaration import status except for distributed ThinLTO under a default-off new option #88024
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
…eSummary::GVFlags - This change doesn't set the bit in the summaries, just make the code changes.
llvmbot
added
LTO
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Mingming Liu (minglotus-6) ChangesThe goal is to generate Implementation notes:
Patch is 43.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88024.diff 12 Files Affected:
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index 66209b8cf3ea2d..29d13cf4f4fae6 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -585,7 +585,11 @@ class GlobalValueSummary {
return Flags.ImportType == GlobalValueSummary::ImportKind::Declaration;
}
- void setImportKind(ImportKind IK) { Flags.ImportType = IK; }
+ GlobalValueSummary::ImportKind importType() const {
+ return static_cast<ImportKind>(Flags.ImportType);
+ }
+
+ void setImportType(ImportKind IK) { Flags.ImportType = IK; }
GlobalValue::VisibilityTypes getVisibility() const {
return (GlobalValue::VisibilityTypes)Flags.Visibility;
diff --git a/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h b/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
index c450acda82ad06..6e197ae2f279c9 100644
--- a/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
+++ b/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
@@ -276,7 +276,8 @@ class ThinLTOCodeGenerator {
void gatherImportedSummariesForModule(
Module &Module, ModuleSummaryIndex &Index,
std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex,
- const lto::InputFile &File);
+ const lto::InputFile &File,
+ ModuleToGVSummaryPtrSet &ModuleToDecSummaries);
/**
* Perform internalization. Index is updated to reflect linkage changes.
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index c4d19e8641eca2..fb38c68b56df0e 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:
- /// 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 functions to import from a source module and their import type.
+ using FunctionsToImportTy =
+ DenseMap<GlobalValue::GUID, GlobalValueSummary::ImportKind>;
/// The different reasons selectCallee will chose not to import a
/// candidate.
@@ -99,8 +99,12 @@ class FunctionImporter {
/// index's module path string table).
using ImportMapTy = DenseMap<StringRef, FunctionsToImportTy>;
- /// The set contains an entry for every global value the module exports.
- using ExportSetTy = DenseSet<ValueInfo>;
+ /// 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.
+ using ExportSetTy = DenseMap<ValueInfo, GlobalValueSummary::ImportKind>;
/// A function of this type is used to load modules referenced by the index.
using ModuleLoaderTy =
@@ -207,11 +211,16 @@ bool convertToDeclaration(GlobalValue &GV);
/// \p ModuleToSummariesForIndex will be populated with the needed summaries
/// from each required module path. Use a std::map instead of StringMap to get
/// stable order for bitcode emission.
+///
+/// \p ModuleToDecSummaries will be populated with the set of declarations \p
+/// ModulePath need from other modules. They key is module path, and the value
+/// is a set of summary pointers.
void gatherImportedSummariesForModule(
StringRef ModulePath,
const DenseMap<StringRef, GVSummaryMapTy> &ModuleToDefinedGVSummaries,
const FunctionImporter::ImportMapTy &ImportList,
- std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex);
+ std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex,
+ ModuleToGVSummaryPtrSet &ModuleToDecSummaries);
/// Emit into \p OutputFilename the files module \p ModulePath will import from.
std::error_code EmitImportsFiles(
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 53060df7f503e0..e7e5a6ab392774 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -158,7 +158,7 @@ void llvm::computeLTOCacheKey(
std::vector<uint64_t> ExportsGUID;
ExportsGUID.reserve(ExportList.size());
- for (const auto &VI : ExportList) {
+ for (const auto &[VI, UnusedImportType] : ExportList) {
auto GUID = VI.getGUID();
ExportsGUID.push_back(GUID);
}
@@ -204,8 +204,8 @@ void llvm::computeLTOCacheKey(
Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
AddUint64(Entry.getFunctions().size());
- for (auto &Fn : Entry.getFunctions())
- AddUint64(Fn);
+ for (auto &[GUID, UnusedImportType] : Entry.getFunctions())
+ AddUint64(GUID);
}
// Include the hash for the resolved ODR.
@@ -275,9 +275,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 &ImpF : ImpM.getFunctions()) {
+ for (auto &[GUID, UnusedImportType] : ImpM.getFunctions()) {
GlobalValueSummary *S =
- Index.findSummaryInModule(ImpF, ImpM.getIdentifier());
+ Index.findSummaryInModule(GUID, ImpM.getIdentifier());
AddUsedThings(S);
// If this is an alias, we also care about any types/etc. that the aliasee
// may reference.
@@ -1389,15 +1389,19 @@ class lto::ThinBackendProc {
llvm::StringRef ModulePath,
const std::string &NewModulePath) {
std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
+ ModuleToGVSummaryPtrSet ModuleToDeclarationSummaries;
std::error_code EC;
gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
- ImportList, ModuleToSummariesForIndex);
+ ImportList, ModuleToSummariesForIndex,
+ ModuleToDeclarationSummaries);
raw_fd_ostream OS(NewModulePath + ".thinlto.bc", EC,
sys::fs::OpenFlags::OF_None);
if (EC)
return errorCodeToError(EC);
- writeIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex);
+
+ writeIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex,
+ &ModuleToDeclarationSummaries);
if (ShouldEmitImportsFiles) {
EC = EmitImportsFiles(ModulePath, NewModulePath + ".imports",
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index 71e8849dc3cc91..a2cd672e0f0a18 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -716,7 +716,14 @@ bool lto::initImportList(const Module &M,
if (Summary->modulePath() == M.getModuleIdentifier())
continue;
// Add an entry to provoke importing by thinBackend.
- ImportList[Summary->modulePath()].insert(GUID);
+ // 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());
}
}
return true;
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index 8f517eb50dc76f..1227f26cc42805 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -766,7 +766,7 @@ void ThinLTOCodeGenerator::crossModuleImport(Module &TheModule,
void ThinLTOCodeGenerator::gatherImportedSummariesForModule(
Module &TheModule, ModuleSummaryIndex &Index,
std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex,
- const lto::InputFile &File) {
+ const lto::InputFile &File, ModuleToGVSummaryPtrSet &ModuleToDecSummaries) {
auto ModuleCount = Index.modulePaths().size();
auto ModuleIdentifier = TheModule.getModuleIdentifier();
@@ -796,7 +796,8 @@ void ThinLTOCodeGenerator::gatherImportedSummariesForModule(
llvm::gatherImportedSummariesForModule(
ModuleIdentifier, ModuleToDefinedGVSummaries,
- ImportLists[ModuleIdentifier], ModuleToSummariesForIndex);
+ ImportLists[ModuleIdentifier], ModuleToSummariesForIndex,
+ ModuleToDecSummaries);
}
/**
@@ -833,9 +834,15 @@ void ThinLTOCodeGenerator::emitImports(Module &TheModule, StringRef OutputName,
ExportLists);
std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
+ // 'EmitImportsFiles' emits the list of modules from which to import from, and
+ // the set of keys in `ModuleToSummariesForIndex` should be a superset of keys
+ // in `ModuleToDecSummaries`, so no need to use `ModuleToDecSummaries` in
+ // `EmitImportFiles`.
+ ModuleToGVSummaryPtrSet ModuleToDecSummaries;
llvm::gatherImportedSummariesForModule(
ModuleIdentifier, ModuleToDefinedGVSummaries,
- ImportLists[ModuleIdentifier], ModuleToSummariesForIndex);
+ ImportLists[ModuleIdentifier], ModuleToSummariesForIndex,
+ ModuleToDecSummaries);
std::error_code EC;
if ((EC = EmitImportsFiles(ModuleIdentifier, OutputName,
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 68f9799616ae6d..77d644dc97ab7d 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -140,6 +140,17 @@ 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
@@ -245,8 +256,10 @@ static auto qualifyCalleeCandidates(
}
/// Given a list of possible callee implementation for a call site, select one
-/// 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).
+/// 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). If caller wants to select eligible
+/// summary
///
/// FIXME: select "best" instead of first that fits. But what is "best"?
/// - The smallest: more likely to be inlined.
@@ -259,24 +272,32 @@ 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 if we can't inline it anyway.
+ // Don't bother importing the definition if we can't inline it anyway.
if (Summary->fflags().NoInline && !ForceImportAll) {
+ TooLargeOrNoInlineSummary = Summary;
Reason = FunctionImporter::ImportFailureReason::NoInline;
continue;
}
@@ -358,17 +379,28 @@ class GlobalsImporter final {
if (!GVS || !Index.canImportGlobalVar(GVS, /* AnalyzeRefs */ true) ||
LocalNotInModule(GVS))
continue;
- auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
+
+ // 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);
// Only update stat and exports if we haven't already imported this
// variable.
- if (!ILI.second)
+ if (!Inserted) {
+ // FIXME: Introduce a wrapper struct around ImportType, and provide
+ // an `updateType` method for better readability, just like how we
+ // update the hotness of a call edge.
+ Iter->second = std::min(GlobalValueSummary::Definition, Iter->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()].insert(VI);
+ (*ExportLists)[RefSummary->modulePath()][VI] =
+ GlobalValueSummary::Definition;
// If variable is not writeonly we attempt to recursively analyze
// its references in order to import referenced constants.
@@ -545,10 +577,11 @@ class WorkloadImportsManager : public ModuleImportsManager {
LLVM_DEBUG(dbgs() << "[Workload][Including]" << VI.name() << " from "
<< ExportingModule << " : "
<< Function::getGUID(VI.name()) << "\n");
- ImportList[ExportingModule].insert(VI.getGUID());
+ ImportList[ExportingModule][VI.getGUID()] =
+ GlobalValueSummary::Definition;
GVI.onImportingSummary(*GVS);
if (ExportLists)
- (*ExportLists)[ExportingModule].insert(VI);
+ (*ExportLists)[ExportingModule][VI] = GlobalValueSummary::Definition;
}
LLVM_DEBUG(dbgs() << "[Workload] Done\n");
}
@@ -769,9 +802,28 @@ static void computeImportForFunction(
}
FunctionImporter::ImportFailureReason Reason{};
- CalleeSummary = selectCallee(Index, VI.getSummaryList(), NewThreshold,
- Summary.modulePath(), Reason);
+
+ // `SummaryForDeclImport` is an summary eligible for declaration import.
+ const GlobalValueSummary *SummaryForDeclImport = nullptr;
+ CalleeSummary =
+ selectCallee(Index, VI.getSummaryList(), NewThreshold,
+ Summary.modulePath(), SummaryForDeclImport, 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.
@@ -791,6 +843,7 @@ static void computeImportForFunction(
FailureInfo = std::make_unique<FunctionImporter::ImportFailureInfo>(
VI, Edge.second.getHotness(), Reason, 1);
}
+
if (ForceImportAll) {
std::string Msg = std::string("Failed to import function ") +
VI.name().str() + " due to " +
@@ -816,11 +869,15 @@ static void computeImportForFunction(
"selectCallee() didn't honor the threshold");
auto ExportModulePath = ResolvedCalleeSummary->modulePath();
- auto ILI = ImportList[ExportModulePath].insert(VI.getGUID());
+
+ // Try emplace the definition entry, and update stats based on insertion
+ // status.
+ auto [Iter, Inserted] = ImportList[ExportModulePath].try_emplace(
+ VI.getGUID(), GlobalValueSummary::Definition);
+
// We previously decided to import this GUID definition if it was already
// inserted in the set of imports from the exporting module.
- bool PreviouslyImported = !ILI.second;
- if (!PreviouslyImported) {
+ if (Inserted || Iter->second == GlobalValueSummary::Declaration) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
@@ -828,11 +885,14 @@ 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].insert(VI);
+ (*ExportLists)[ExportModulePath][VI] = GlobalValueSummary::Definition;
}
auto GetAdjustedThreshold = [](unsigned Threshold, bool IsHotCallsite) {
@@ -939,12 +999,20 @@ static bool isGlobalVarSummary(const ModuleSummaryIndex &Index,
}
template <class T>
-static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index,
- T &Cont) {
+static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, T &Cont,
+ unsigned &DefinedGVS,
+ unsigned &DefinedFS) {
unsigned NumGVS = 0;
- for (auto &V : Cont)
- if (isGlobalVarSummary(Index, V))
+ DefinedGVS = 0;
+ DefinedFS = 0;
+ for (auto &[GUID, Type] : Cont) {
+ if (isGlobalVarSummary(Index, GUID)) {
+ if (Type == GlobalValueSummary::Definition)
+ ++DefinedGVS;
++NumGVS;
+ } else if (Type == GlobalValueSummary::Definition)
+ ++DefinedFS;
+ }
return NumGVS;
}
#endif
@@ -954,13 +1022,12 @@ 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)
- FlattenedImports.insert(ExportPerModule.second.begin(),
- ExportPerModule.second.end());
+ for (auto &[GUID, Type] : ExportPerModule.second)
+ FlattenedImports.insert(GUID);
// Checks that all GUIDs of read/writeonly var...
[truncated]
|
|
@llvm/pr-subscribers-lto Author: Mingming Liu (minglotus-6) ChangesThe goal is to generate Implementation notes:
Patch is 43.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88024.diff 12 Files Affected:
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index 66209b8cf3ea2d..29d13cf4f4fae6 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -585,7 +585,11 @@ class GlobalValueSummary {
return Flags.ImportType == GlobalValueSummary::ImportKind::Declaration;
}
- void setImportKind(ImportKind IK) { Flags.ImportType = IK; }
+ GlobalValueSummary::ImportKind importType() const {
+ return static_cast<ImportKind>(Flags.ImportType);
+ }
+
+ void setImportType(ImportKind IK) { Flags.ImportType = IK; }
GlobalValue::VisibilityTypes getVisibility() const {
return (GlobalValue::VisibilityTypes)Flags.Visibility;
diff --git a/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h b/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
index c450acda82ad06..6e197ae2f279c9 100644
--- a/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
+++ b/llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
@@ -276,7 +276,8 @@ class ThinLTOCodeGenerator {
void gatherImportedSummariesForModule(
Module &Module, ModuleSummaryIndex &Index,
std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex,
- const lto::InputFile &File);
+ const lto::InputFile &File,
+ ModuleToGVSummaryPtrSet &ModuleToDecSummaries);
/**
* Perform internalization. Index is updated to reflect linkage changes.
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index c4d19e8641eca2..fb38c68b56df0e 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:
- /// 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 functions to import from a source module and their import type.
+ using FunctionsToImportTy =
+ DenseMap<GlobalValue::GUID, GlobalValueSummary::ImportKind>;
/// The different reasons selectCallee will chose not to import a
/// candidate.
@@ -99,8 +99,12 @@ class FunctionImporter {
/// index's module path string table).
using ImportMapTy = DenseMap<StringRef, FunctionsToImportTy>;
- /// The set contains an entry for every global value the module exports.
- using ExportSetTy = DenseSet<ValueInfo>;
+ /// 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.
+ using ExportSetTy = DenseMap<ValueInfo, GlobalValueSummary::ImportKind>;
/// A function of this type is used to load modules referenced by the index.
using ModuleLoaderTy =
@@ -207,11 +211,16 @@ bool convertToDeclaration(GlobalValue &GV);
/// \p ModuleToSummariesForIndex will be populated with the needed summaries
/// from each required module path. Use a std::map instead of StringMap to get
/// stable order for bitcode emission.
+///
+/// \p ModuleToDecSummaries will be populated with the set of declarations \p
+/// ModulePath need from other modules. They key is module path, and the value
+/// is a set of summary pointers.
void gatherImportedSummariesForModule(
StringRef ModulePath,
const DenseMap<StringRef, GVSummaryMapTy> &ModuleToDefinedGVSummaries,
const FunctionImporter::ImportMapTy &ImportList,
- std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex);
+ std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex,
+ ModuleToGVSummaryPtrSet &ModuleToDecSummaries);
/// Emit into \p OutputFilename the files module \p ModulePath will import from.
std::error_code EmitImportsFiles(
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 53060df7f503e0..e7e5a6ab392774 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -158,7 +158,7 @@ void llvm::computeLTOCacheKey(
std::vector<uint64_t> ExportsGUID;
ExportsGUID.reserve(ExportList.size());
- for (const auto &VI : ExportList) {
+ for (const auto &[VI, UnusedImportType] : ExportList) {
auto GUID = VI.getGUID();
ExportsGUID.push_back(GUID);
}
@@ -204,8 +204,8 @@ void llvm::computeLTOCacheKey(
Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
AddUint64(Entry.getFunctions().size());
- for (auto &Fn : Entry.getFunctions())
- AddUint64(Fn);
+ for (auto &[GUID, UnusedImportType] : Entry.getFunctions())
+ AddUint64(GUID);
}
// Include the hash for the resolved ODR.
@@ -275,9 +275,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 &ImpF : ImpM.getFunctions()) {
+ for (auto &[GUID, UnusedImportType] : ImpM.getFunctions()) {
GlobalValueSummary *S =
- Index.findSummaryInModule(ImpF, ImpM.getIdentifier());
+ Index.findSummaryInModule(GUID, ImpM.getIdentifier());
AddUsedThings(S);
// If this is an alias, we also care about any types/etc. that the aliasee
// may reference.
@@ -1389,15 +1389,19 @@ class lto::ThinBackendProc {
llvm::StringRef ModulePath,
const std::string &NewModulePath) {
std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
+ ModuleToGVSummaryPtrSet ModuleToDeclarationSummaries;
std::error_code EC;
gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
- ImportList, ModuleToSummariesForIndex);
+ ImportList, ModuleToSummariesForIndex,
+ ModuleToDeclarationSummaries);
raw_fd_ostream OS(NewModulePath + ".thinlto.bc", EC,
sys::fs::OpenFlags::OF_None);
if (EC)
return errorCodeToError(EC);
- writeIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex);
+
+ writeIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex,
+ &ModuleToDeclarationSummaries);
if (ShouldEmitImportsFiles) {
EC = EmitImportsFiles(ModulePath, NewModulePath + ".imports",
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index 71e8849dc3cc91..a2cd672e0f0a18 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -716,7 +716,14 @@ bool lto::initImportList(const Module &M,
if (Summary->modulePath() == M.getModuleIdentifier())
continue;
// Add an entry to provoke importing by thinBackend.
- ImportList[Summary->modulePath()].insert(GUID);
+ // 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());
}
}
return true;
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index 8f517eb50dc76f..1227f26cc42805 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -766,7 +766,7 @@ void ThinLTOCodeGenerator::crossModuleImport(Module &TheModule,
void ThinLTOCodeGenerator::gatherImportedSummariesForModule(
Module &TheModule, ModuleSummaryIndex &Index,
std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex,
- const lto::InputFile &File) {
+ const lto::InputFile &File, ModuleToGVSummaryPtrSet &ModuleToDecSummaries) {
auto ModuleCount = Index.modulePaths().size();
auto ModuleIdentifier = TheModule.getModuleIdentifier();
@@ -796,7 +796,8 @@ void ThinLTOCodeGenerator::gatherImportedSummariesForModule(
llvm::gatherImportedSummariesForModule(
ModuleIdentifier, ModuleToDefinedGVSummaries,
- ImportLists[ModuleIdentifier], ModuleToSummariesForIndex);
+ ImportLists[ModuleIdentifier], ModuleToSummariesForIndex,
+ ModuleToDecSummaries);
}
/**
@@ -833,9 +834,15 @@ void ThinLTOCodeGenerator::emitImports(Module &TheModule, StringRef OutputName,
ExportLists);
std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
+ // 'EmitImportsFiles' emits the list of modules from which to import from, and
+ // the set of keys in `ModuleToSummariesForIndex` should be a superset of keys
+ // in `ModuleToDecSummaries`, so no need to use `ModuleToDecSummaries` in
+ // `EmitImportFiles`.
+ ModuleToGVSummaryPtrSet ModuleToDecSummaries;
llvm::gatherImportedSummariesForModule(
ModuleIdentifier, ModuleToDefinedGVSummaries,
- ImportLists[ModuleIdentifier], ModuleToSummariesForIndex);
+ ImportLists[ModuleIdentifier], ModuleToSummariesForIndex,
+ ModuleToDecSummaries);
std::error_code EC;
if ((EC = EmitImportsFiles(ModuleIdentifier, OutputName,
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 68f9799616ae6d..77d644dc97ab7d 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -140,6 +140,17 @@ 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
@@ -245,8 +256,10 @@ static auto qualifyCalleeCandidates(
}
/// Given a list of possible callee implementation for a call site, select one
-/// 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).
+/// 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). If caller wants to select eligible
+/// summary
///
/// FIXME: select "best" instead of first that fits. But what is "best"?
/// - The smallest: more likely to be inlined.
@@ -259,24 +272,32 @@ 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 if we can't inline it anyway.
+ // Don't bother importing the definition if we can't inline it anyway.
if (Summary->fflags().NoInline && !ForceImportAll) {
+ TooLargeOrNoInlineSummary = Summary;
Reason = FunctionImporter::ImportFailureReason::NoInline;
continue;
}
@@ -358,17 +379,28 @@ class GlobalsImporter final {
if (!GVS || !Index.canImportGlobalVar(GVS, /* AnalyzeRefs */ true) ||
LocalNotInModule(GVS))
continue;
- auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
+
+ // 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);
// Only update stat and exports if we haven't already imported this
// variable.
- if (!ILI.second)
+ if (!Inserted) {
+ // FIXME: Introduce a wrapper struct around ImportType, and provide
+ // an `updateType` method for better readability, just like how we
+ // update the hotness of a call edge.
+ Iter->second = std::min(GlobalValueSummary::Definition, Iter->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()].insert(VI);
+ (*ExportLists)[RefSummary->modulePath()][VI] =
+ GlobalValueSummary::Definition;
// If variable is not writeonly we attempt to recursively analyze
// its references in order to import referenced constants.
@@ -545,10 +577,11 @@ class WorkloadImportsManager : public ModuleImportsManager {
LLVM_DEBUG(dbgs() << "[Workload][Including]" << VI.name() << " from "
<< ExportingModule << " : "
<< Function::getGUID(VI.name()) << "\n");
- ImportList[ExportingModule].insert(VI.getGUID());
+ ImportList[ExportingModule][VI.getGUID()] =
+ GlobalValueSummary::Definition;
GVI.onImportingSummary(*GVS);
if (ExportLists)
- (*ExportLists)[ExportingModule].insert(VI);
+ (*ExportLists)[ExportingModule][VI] = GlobalValueSummary::Definition;
}
LLVM_DEBUG(dbgs() << "[Workload] Done\n");
}
@@ -769,9 +802,28 @@ static void computeImportForFunction(
}
FunctionImporter::ImportFailureReason Reason{};
- CalleeSummary = selectCallee(Index, VI.getSummaryList(), NewThreshold,
- Summary.modulePath(), Reason);
+
+ // `SummaryForDeclImport` is an summary eligible for declaration import.
+ const GlobalValueSummary *SummaryForDeclImport = nullptr;
+ CalleeSummary =
+ selectCallee(Index, VI.getSummaryList(), NewThreshold,
+ Summary.modulePath(), SummaryForDeclImport, 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.
@@ -791,6 +843,7 @@ static void computeImportForFunction(
FailureInfo = std::make_unique<FunctionImporter::ImportFailureInfo>(
VI, Edge.second.getHotness(), Reason, 1);
}
+
if (ForceImportAll) {
std::string Msg = std::string("Failed to import function ") +
VI.name().str() + " due to " +
@@ -816,11 +869,15 @@ static void computeImportForFunction(
"selectCallee() didn't honor the threshold");
auto ExportModulePath = ResolvedCalleeSummary->modulePath();
- auto ILI = ImportList[ExportModulePath].insert(VI.getGUID());
+
+ // Try emplace the definition entry, and update stats based on insertion
+ // status.
+ auto [Iter, Inserted] = ImportList[ExportModulePath].try_emplace(
+ VI.getGUID(), GlobalValueSummary::Definition);
+
// We previously decided to import this GUID definition if it was already
// inserted in the set of imports from the exporting module.
- bool PreviouslyImported = !ILI.second;
- if (!PreviouslyImported) {
+ if (Inserted || Iter->second == GlobalValueSummary::Declaration) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
@@ -828,11 +885,14 @@ 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].insert(VI);
+ (*ExportLists)[ExportModulePath][VI] = GlobalValueSummary::Definition;
}
auto GetAdjustedThreshold = [](unsigned Threshold, bool IsHotCallsite) {
@@ -939,12 +999,20 @@ static bool isGlobalVarSummary(const ModuleSummaryIndex &Index,
}
template <class T>
-static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index,
- T &Cont) {
+static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, T &Cont,
+ unsigned &DefinedGVS,
+ unsigned &DefinedFS) {
unsigned NumGVS = 0;
- for (auto &V : Cont)
- if (isGlobalVarSummary(Index, V))
+ DefinedGVS = 0;
+ DefinedFS = 0;
+ for (auto &[GUID, Type] : Cont) {
+ if (isGlobalVarSummary(Index, GUID)) {
+ if (Type == GlobalValueSummary::Definition)
+ ++DefinedGVS;
++NumGVS;
+ } else if (Type == GlobalValueSummary::Definition)
+ ++DefinedFS;
+ }
return NumGVS;
}
#endif
@@ -954,13 +1022,12 @@ 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)
- FlattenedImports.insert(ExportPerModule.second.begin(),
- ExportPerModule.second.end());
+ for (auto &[GUID, Type] : ExportPerModule.second)
+ FlattenedImports.insert(GUID);
// Checks that all GUIDs of read/writeonly var...
[truncated]
|
|
Tagging @jvoung since I cannot add you into reviewer. |
minglotus-6
changed the title
|
#87600 is a functional change and the diffbase of this patch, and In the diffbase, bitcode writer takes maps as additional parameters to populate import status, and it's not straightforward to construct regression tests there without this patch. I wonder if I shall introduce |
minglotus-6
commented
May 11, 2024
| @@ -276,7 +276,7 @@ class ThinLTOCodeGenerator { | |||
| void gatherImportedSummariesForModule( | |||
| Module &Module, ModuleSummaryIndex &Index, | |||
| std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex, | |||
| const lto::InputFile &File); | |||
| const lto::InputFile &File, GVSummaryPtrSet &DecSummaries); | |||
There was a problem hiding this comment.
This makes sense. I'll do this in #87600 mostly because this patch doesn't use DecSummaries.
| @@ -207,11 +211,16 @@ bool convertToDeclaration(GlobalValue &GV); | |||
| /// \p ModuleToSummariesForIndex will be populated with the needed summaries | |||
| /// from each required module path. Use a std::map instead of StringMap to get | |||
| /// stable order for bitcode emission. | |||
| /// | |||
| /// \p ModuleToDecSummaries will be populated with the set of declarations \p | |||
| /// ModulePath need from other modules. They key is module path, and the value | |||
There was a problem hiding this comment.
thanks for the catch. I'll update gatherImportedSummariesForModule in the next change (#87600) mostly because DecSummaries is used by bitcode writer.
| @@ -833,9 +833,14 @@ void ThinLTOCodeGenerator::emitImports(Module &TheModule, StringRef OutputName, | |||
| ExportLists); | |||
|
|
|||
| std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex; | |||
| // 'EmitImportsFiles' emits the list of modules from which to import from, and | |||
| // the set of keys in `ModuleToSummariesForIndex` should be a superset of keys | |||
| // in `ModuleToDecSummaries`, so no need to use `ModuleToDecSummaries` in | |||
There was a problem hiding this comment.
As noted in the other two comments I'll do this in the next patch.
| if (Import) { | ||
| auto ImportType = maybeGetImportType(ImportGUIDs, GUID); | ||
|
|
||
| if (!ImportType) { |
There was a problem hiding this comment.
Added 'declarations' into debug message and kept the similar flow as before (no new continue path).
Added IMPORTDUMP: lines to regression test.
| << SrcModule->getSourceFileName() << "\n"); | ||
| if (Import) { | ||
| auto ImportType = maybeGetImportType(ImportGUIDs, GUID); | ||
| if (!ImportType) |
llvm/lib/LTO/LTO.cpp
Outdated
| @@ -158,7 +158,7 @@ void llvm::computeLTOCacheKey( | |||
|
|
|||
| std::vector<uint64_t> ExportsGUID; | |||
| ExportsGUID.reserve(ExportList.size()); | |||
| for (const auto &VI : ExportList) { | |||
| for (const auto &[VI, UnusedImportType] : ExportList) { | |||
There was a problem hiding this comment.
done for ExportList and ImportList.
This way of splitting makes a lot of sense. For in-process ThinLTO, |
minglotus-6
changed the title
jvoung
approved these changes
May 15, 2024
llvm/lib/LTO/LTO.cpp
Outdated
| @@ -121,6 +121,9 @@ void llvm::computeLTOCacheKey( | |||
| support::endian::write64le(Data, I); | |||
| Hasher.update(Data); | |||
| }; | |||
| auto AddUint8 = [&](const uint8_t &I) { | |||
There was a problem hiding this comment.
nit: could pass by value, since uint8_t is small (similar to AddUnsigned)?
| // module. We do this after the above insertion since we may hit the same | ||
| // ref/call target multiple times in above loop, and it is more efficient to | ||
| // avoid a set lookup each time. | ||
| // Prune list computed above to only include values defined in the |
There was a problem hiding this comment.
nit: can keep the original single space in the comment?
teresajohnson
approved these changes
May 16, 2024
|
|
||
| ; The following information are printed from `FunctionImporter::importFunctions` | ||
| ; in the postlink pipeline. | ||
| ; TODO: Update debug information for postlink pipeline. |
There was a problem hiding this comment.
I'm not sure I understand the TODO?
There was a problem hiding this comment.
I'm thinking about printing counters for both definitions and declarations around these lines https://github.com/minglotus-6/llvm-project/blob/f72611b40de8c060c3901dab0ee9f8424dd3012b/llvm/lib/Transforms/IPO/FunctionImport.cpp#L1757-L1762
I removed the comments in this test case and added TODO in the source code to make it clearer.
| ; IMPORT-DAG: define available_externally void @small_func | ||
| ; IMPORT-DAG: define available_externally hidden void @small_indirect_callee | ||
| ; IMPORT-DAG: declare void @large_func | ||
| ; IMPORT-NOT: large_indirect_callee |
There was a problem hiding this comment.
Maybe add a TODO or other comment that this is temporary until the corresponding change to do the actual declaration import goes in?
There was a problem hiding this comment.
sounds good. I somehow didn't find the existingTODO when we discussed the follow-up plans (import declaration implementation and test in my llvm fork), and yesterday I thought I deleted them mistakenly when updating PRs.
Turns out there are two TODOs on line 25 and line 57, so gathered them at line 83 - line 85.
|
I had tested the proof-of-concept (along with #87600 and declaration import) on internal large workloads. I'll do a sanity check with the reviewed one and land this after that. |
minglotus-6
added a commit
that referenced
this pull request
May 19, 2024
Add 'sort' here since it's helpful when container type changes (for example, #88024 wants to change container type from `unordered_set` to `DenseMap) @MaskRay points out `std::` doesn't randomize the iteration order of `unordered_{set,map}`, and the iteration order for single build is deterministic.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
minglotus-6
added a commit
that referenced
this pull request
May 20, 2024
…ibuted ThinLTO under a default-off new option" (#92715) Reverts #88024 Build bot failures (https://lab.llvm.org/buildbot/#/builders/259/builds/4727 and https://lab.llvm.org/buildbot/#/builders/9/builds/43876)
minglotus-6
added a commit
that referenced
this pull request
May 20, 2024
…ibuted ThinLTO under a default-off new option" (#92718) The original PR is reviewed in #88024, and this PR adds one line (b9f04d1) to fix test Limit to one thread for in-process ThinLTO to test `LLVM_DEBUG` log. - This should fix build bot failure like https://lab.llvm.org/buildbot/#/builders/259/builds/4727 and https://lab.llvm.org/buildbot/#/builders/9/builds/43876 - I could repro the failure and see interleaved log messages by using `-thinlto-threads=all` **Original Commit Message:** The goal is to populate `declaration` import status if a new flag `-import-declaration` is on. * For in-process ThinLTO, the `declaration` status is visible to backend `function-import` pass, so `FunctionImporter::importFunctions` should read the import status and be no-op for declaration summaries. Basically, the postlink pipeline is updated to keep its current behavior (import definitions), but not updated to handle `declaration` summaries. Two use cases ([better call-graph sort](https://discourse.llvm.org/t/rfc-for-better-call-graph-sort-build-a-more-complete-call-graph-by-adding-more-indirect-call-edges/74029#support-cross-module-function-declaration-import-5) or [cross-module auto-init](#87597 (comment))) would use this bit differently. * For distributed ThinLTO, the `declaration` status is not serialized to bitcode. As discussed, #87600 will do this.
|
FWIW, if I remember correctly from trying some versions of this, I think DenseMap for |
Thanks for this info! To optimize the memory usage of this patch, I'm thinking about to storing per-module imported declaration set as another Note this will duplicate module paths ( |
This would only duplicate the StringRef, not the whole string though I think? The module path strings should only be stored in the ModulePathStringTable.
|
Yes. I understand that |
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.
The goal is to populate
declarationimport status if a new flag-import-declarationis on.For in-process ThinLTO, the
declarationstatus is visible to backendfunction-importpass, soFunctionImporter::importFunctionsshould read the import status and be no-op for declaration summaries. Basically, the postlink pipeline is updated to keep its current behavior (import definitions), but not updated to handledeclarationsummaries. Two use cases (better call-graph sort or cross-module auto-init) would use this bit differently.For distributed ThinLTO, the
declarationstatus is not serialized to bitcode. As discussed, [ThinLTO][Bitcode] Generate import type in bitcode #87600 will do this.