[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -207,11 +211,16 @@ bool convertToDeclaration(GlobalValue ); /// \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 jvoung wrote: nit: ModuleToDecSummaries -> DecSummaries now and update the "The key is ..." part https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -833,9 +833,14 @@ void ThinLTOCodeGenerator::emitImports(Module , StringRef OutputName, ExportLists); std::map 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 jvoung wrote: nit: update DecSummaries in comment https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -276,7 +276,7 @@ class ThinLTOCodeGenerator {
void gatherImportedSummariesForModule(
Module , ModuleSummaryIndex ,
std::map ,
- const lto::InputFile );
+ const lto::InputFile , GVSummaryPtrSet );
jvoung wrote:
nit: consider keeping the outparams together (so DecSummaries next to
ModuleToSummariesForIndex, and File after?)
https://github.com/llvm/llvm-project/pull/88024
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
teresajohnson wrote: > #87600 is a functional change and the diffbase of this patch, and > `llvm/test/ThinLTO/X86/import_callee_declaration.ll` should be a test case > for both patches. > > In the [diffbase](https://github.com/llvm/llvm-project/pull/87600), 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 `cl::list` in > llvm-lto/llvm-lto2 (as a repeated arg) to specify `filename:GUID` to test the > diffbase alone. Rather than add an option just for testing that one alone, I have a suggestion for splitting up the PRs slightly differently. What if you submitted this one first, minus the modified calls to writeIndexToFile and the part of the test that checks the disassembled index (just have the testing for this one check the number of declarations imported and other debug messages). Then move the modified calls to writeIndexToFile and the index disassembly checking to PR87600 that can be committed as a follow on? That way each change comes with a test. https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -1670,11 +1798,15 @@ Expected FunctionImporter::importFunctions(
if (!GV.hasName())
continue;
auto GUID = GV.getGUID();
- auto Import = ImportGUIDs.count(GUID);
- LLVM_DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing global "
-<< GUID << " " << GV.getName() << " from "
-<< SrcModule->getSourceFileName() << "\n");
- if (Import) {
+ auto ImportType = maybeGetImportType(ImportGUIDs, GUID);
+ if (!ImportType)
teresajohnson wrote:
Or do what I suggested above which goes back to only needing one LLVM_DEBUG
https://github.com/llvm/llvm-project/pull/88024
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -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 teresajohnson wrote: dangling sentence? https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
https://github.com/teresajohnson commented: I only had time for a cursory review, some comments / suggestions below. I also have a suggestion for the testing issue wrt to the other patch, will note that separately https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -1634,17 +1752,27 @@ Expected FunctionImporter::importFunctions(
return std::move(Err);
auto = FunctionsToImportPerModule->second;
+
// Find the globals to import
SetVector GlobalsToImport;
for (Function : *SrcModule) {
if (!F.hasName())
continue;
auto GUID = F.getGUID();
- auto Import = ImportGUIDs.count(GUID);
- LLVM_DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing function "
-<< GUID << " " << F.getName() << " from "
-<< SrcModule->getSourceFileName() << "\n");
- if (Import) {
+ auto ImportType = maybeGetImportType(ImportGUIDs, GUID);
+
+ if (!ImportType) {
teresajohnson wrote:
You could combine this with the below ImportDefinition checking to keep the
same flow as before with one debug message, e.g.:
```
auto ImportType = maybeGetImportType(...);
auto ImportDefinition = false;
if (ImportType) {
ImportDefinition = ...;
}
LLVM_DEBUG(dbgs() << (ImportDefinition ...
if (ImportDefinition) {
...
```
https://github.com/llvm/llvm-project/pull/88024
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -1634,17 +1752,27 @@ Expected FunctionImporter::importFunctions(
return std::move(Err);
auto = FunctionsToImportPerModule->second;
+
// Find the globals to import
SetVector GlobalsToImport;
for (Function : *SrcModule) {
if (!F.hasName())
continue;
auto GUID = F.getGUID();
- auto Import = ImportGUIDs.count(GUID);
- LLVM_DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing function "
-<< GUID << " " << F.getName() << " from "
-<< SrcModule->getSourceFileName() << "\n");
- if (Import) {
+ auto ImportType = maybeGetImportType(ImportGUIDs, GUID);
+
+ if (!ImportType) {
teresajohnson wrote:
Also consider indicating which are imported as declarations in the debug
message?
https://github.com/llvm/llvm-project/pull/88024
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -158,7 +158,7 @@ void llvm::computeLTOCacheKey(
std::vector ExportsGUID;
ExportsGUID.reserve(ExportList.size());
- for (const auto : ExportList) {
+ for (const auto &[VI, UnusedImportType] : ExportList) {
teresajohnson wrote:
We should probably include the new import type result in the cache key. Because
if that changes then presumably the cached object should be invalidated as it
would be different?
https://github.com/llvm/llvm-project/pull/88024
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
https://github.com/teresajohnson edited https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
@@ -1670,11 +1798,15 @@ Expected FunctionImporter::importFunctions(
if (!GV.hasName())
continue;
auto GUID = GV.getGUID();
- auto Import = ImportGUIDs.count(GUID);
- LLVM_DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing global "
-<< GUID << " " << GV.getName() << " from "
-<< SrcModule->getSourceFileName() << "\n");
- if (Import) {
+ auto ImportType = maybeGetImportType(ImportGUIDs, GUID);
+ if (!ImportType)
teresajohnson wrote:
Do we need to emit a debug message in this case like you are doing for
functions above? Ditto for aliases below
https://github.com/llvm/llvm-project/pull/88024
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
https://github.com/minglotus-6 updated
https://github.com/llvm/llvm-project/pull/88024
>From cfb63d775d43a28b560d938346f1dd4b2dddc765 Mon Sep 17 00:00:00 2001
From: mingmingl
Date: Thu, 4 Apr 2024 11:54:17 -0700
Subject: [PATCH 1/9] function import changes
---
llvm/include/llvm/IR/ModuleSummaryIndex.h | 24
.../llvm/Transforms/IPO/FunctionImport.h | 18 ++-
llvm/lib/LTO/LTO.cpp | 13 +-
llvm/lib/LTO/LTOBackend.cpp | 5 +-
llvm/lib/LTO/ThinLTOCodeGenerator.cpp | 9 +-
llvm/lib/Transforms/IPO/FunctionImport.cpp| 130 --
llvm/tools/llvm-link/llvm-link.cpp| 2 +-
7 files changed, 146 insertions(+), 55 deletions(-)
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h
b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index 286b51bda0e2..259fe56ce5f6 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -296,6 +296,30 @@ template <> struct DenseMapInfo {
static unsigned getHashValue(ValueInfo I) { return (uintptr_t)I.getRef(); }
};
+struct SummaryImportInfo {
+ enum class ImportType : uint8_t {
+NotImported = 0,
+Declaration = 1,
+Definition = 2,
+ };
+ unsigned Type : 3;
+ SummaryImportInfo() : Type(static_cast(ImportType::NotImported)) {}
+ SummaryImportInfo(ImportType Type) : Type(static_cast(Type)) {}
+
+ // FIXME: delete the first two set* helper function.
+ void updateType(ImportType InputType) {
+Type = std::max(Type, static_cast(InputType));
+ }
+
+ bool isDefinition() const {
+return static_cast(Type) == ImportType::Definition;
+ }
+
+ bool isDeclaration() const {
+return static_cast(Type) == ImportType::Declaration;
+ }
+};
+
/// Summary of memprof callsite metadata.
struct CallsiteInfo {
// Actual callee function.
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index c4d19e8641ec..9adc0c31eed4 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -33,7 +33,14 @@ 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;
+ using FunctionsToImportTy = DenseMap;
+
+ // FIXME: Remove this.
+ enum ImportStatus {
+NotImported,
+ImportDeclaration,
+ImportDefinition,
+ };
/// The different reasons selectCallee will chose not to import a
/// candidate.
@@ -99,8 +106,10 @@ class FunctionImporter {
/// index's module path string table).
using ImportMapTy = DenseMap;
- /// The set contains an entry for every global value the module exports.
- using ExportSetTy = DenseSet;
+ /// The map contains an entry for every global value the module exports, the
+ /// key being the value info, and the value is the summary-based import info.
+ /// FIXME: Does this set need to be a map?
+ using ExportSetTy = DenseMap;
/// A function of this type is used to load modules referenced by the index.
using ModuleLoaderTy =
@@ -211,7 +220,8 @@ void gatherImportedSummariesForModule(
StringRef ModulePath,
const DenseMap ,
const FunctionImporter::ImportMapTy ,
-std::map );
+std::map ,
+ModuleToGVSummaryPtrSet );
/// 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 53060df7f503..ace533fe28c9 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -159,7 +159,7 @@ void llvm::computeLTOCacheKey(
std::vector ExportsGUID;
ExportsGUID.reserve(ExportList.size());
for (const auto : ExportList) {
-auto GUID = VI.getGUID();
+auto GUID = VI.first.getGUID();
ExportsGUID.push_back(GUID);
}
@@ -205,7 +205,7 @@ void llvm::computeLTOCacheKey(
AddUint64(Entry.getFunctions().size());
for (auto : Entry.getFunctions())
- AddUint64(Fn);
+ AddUint64(Fn.first);
}
// Include the hash for the resolved ODR.
@@ -277,7 +277,7 @@ void llvm::computeLTOCacheKey(
for (const ImportModule : ImportModulesVector)
for (auto : ImpM.getFunctions()) {
GlobalValueSummary *S =
- Index.findSummaryInModule(ImpF, ImpM.getIdentifier());
+ Index.findSummaryInModule(ImpF.first, ImpM.getIdentifier());
AddUsedThings(S);
// If this is an alias, we also care about any types/etc. that the
aliasee
// may reference.
@@ -1389,15 +1389,18 @@ class lto::ThinBackendProc {
llvm::StringRef ModulePath,
const std::string ) {
std::map ModuleToSummariesForIndex;
+ModuleToGVSummaryPtrSet ModuleToDeclarationSummaries;
std::error_code EC;
gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
-
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
minglotus-6 wrote: https://github.com/llvm/llvm-project/pull/87600 is a functional change and the diffbase of this patch, and `llvm/test/ThinLTO/X86/import_callee_declaration.ll` should be a test case for both patches. In the [diffbase](https://github.com/llvm/llvm-project/pull/87600), 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 `cl::list` in llvm-lto/llvm-lto2 (as a repeated arg) to specify `filename:GUID` to test the diffbase alone. https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [ThinLTO] Generate import status in per-module combined summary (PR #88024)
https://github.com/minglotus-6 edited https://github.com/llvm/llvm-project/pull/88024 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
