[ThinLTO] Allow importing based on a workload definition #74545
Merged
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
An example of a "workload definition" would be "the transitive closure of functions actually called to satisfy a RPC request", i.e. a (typically significantly) smaller subset of the transitive closure (static + possible indirect call targets) of callees. This means this workload definition is a type of flat dynamic profile. Producing one is not in scope - it can be produced offline from traces, or from sample-based profiles, etc. This patch adds awareness to ThinLTO of such a concept. A workload is defined as a root and a list of functions. All function references are by-name (more readable than GUIDs). In the case of aliases, the expectation is the list contains all the alternative names. The workload definitions are presented to the linker as a json file, containing a dictionary. The keys are the roots, the values are the list of functions. The import list for a module defining a root will be the functions listed for it in the profile. Using names this way assumes unique names for internal functions, i.e. clang's `-funique-internal-linkage-names`. Note that the behavior affects the entire module where a root is defined (i.e. different workloads best be defined in different modules), and does not affect modules that don't define roots.
llvmbot
added
LTO
|
@llvm/pr-subscribers-lto @llvm/pr-subscribers-llvm-transforms Author: Mircea Trofin (mtrofin) ChangesAn example of a "workload definition" would be "the transitive closure of functions actually called to satisfy a RPC request", i.e. a (typically significantly) smaller subset of the transitive closure (static + possible indirect call targets) of callees. This means this workload definition is a type of flat dynamic profile. Producing one is not in scope - it can be produced offline from traces, or from sample-based profiles, etc. This patch adds awareness to ThinLTO of such a concept. A workload is defined as a root and a list of functions. All function references are by-name (more readable than GUIDs). In the case of aliases, the expectation is the list contains all the alternative names. The workload definitions are presented to the linker as a json file, containing a dictionary. The keys are the roots, the values are the list of functions. The import list for a module defining a root will be the functions listed for it in the profile. Using names this way assumes unique names for internal functions, i.e. clang's Note that the behavior affects the entire module where a root is defined (i.e. different workloads best be defined in different modules), and does not affect modules that don't define roots. Full diff: https://github.com/llvm/llvm-project/pull/74545.diff 5 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 9c546b531dff4..bcf141b42be16 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -37,6 +37,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/JSON.h"
#include "llvm/Support/SourceMgr.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Transforms/IPO/Internalize.h"
@@ -138,6 +139,9 @@ static cl::opt<bool>
ImportAllIndex("import-all-index",
cl::desc("Import all external functions in index."));
+static cl::opt<std::string> WorkloadDefinitions("thinlto-workload-def",
+ cl::Hidden);
+
// Load lazily a module from \p FileName in \p Context.
static std::unique_ptr<Module> loadFile(const std::string &FileName,
LLVMContext &Context) {
@@ -369,14 +373,16 @@ class GlobalsImporter final {
}
};
+static const char *getFailureName(FunctionImporter::ImportFailureReason Reason);
+
/// Determine the list of imports and exports for each module.
-class ModuleImportsManager final {
+class ModuleImportsManager {
+protected:
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
IsPrevailing;
const ModuleSummaryIndex &Index;
DenseMap<StringRef, FunctionImporter::ExportSetTy> *const ExportLists;
-public:
ModuleImportsManager(
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
IsPrevailing,
@@ -384,14 +390,221 @@ class ModuleImportsManager final {
DenseMap<StringRef, FunctionImporter::ExportSetTy> *ExportLists = nullptr)
: IsPrevailing(IsPrevailing), Index(Index), ExportLists(ExportLists) {}
+public:
+ virtual ~ModuleImportsManager() = default;
+
/// Given the list of globals defined in a module, compute the list of imports
/// as well as the list of "exports", i.e. the list of symbols referenced from
/// another module (that may require promotion).
- void computeImportForModule(const GVSummaryMapTy &DefinedGVSummaries,
- StringRef ModName,
- FunctionImporter::ImportMapTy &ImportList);
+ virtual void
+ computeImportForModule(const GVSummaryMapTy &DefinedGVSummaries,
+ StringRef ModName,
+ FunctionImporter::ImportMapTy &ImportList);
+
+ static std::unique_ptr<ModuleImportsManager>
+ create(function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+ IsPrevailing,
+ const ModuleSummaryIndex &Index,
+ DenseMap<StringRef, FunctionImporter::ExportSetTy> *ExportLists =
+ nullptr);
};
+class WorkloadImportsManager : public ModuleImportsManager {
+ // Keep a module name -> defined value infos association. We use it to
+ // determine if a module's import list should be done by the base
+ // ModuleImportsManager or by us.
+ StringMap<DenseSet<ValueInfo>> Workloads;
+
+ void
+ computeImportForModule(const GVSummaryMapTy &DefinedGVSummaries,
+ StringRef ModName,
+ FunctionImporter::ImportMapTy &ImportList) override {
+ auto SetIter = Workloads.find(ModName);
+ if (SetIter == Workloads.end()) {
+ LLVM_DEBUG(dbgs() << "[Workload] " << ModName
+ << " does not contain the root of any context.\n");
+ return ModuleImportsManager::computeImportForModule(DefinedGVSummaries,
+ ModName, ImportList);
+ }
+ LLVM_DEBUG(dbgs() << "[Workload] " << ModName
+ << " contains the root(s) of context(s).\n");
+
+ GlobalsImporter GVI(Index, DefinedGVSummaries, IsPrevailing, ImportList,
+ ExportLists);
+ auto &ValueInfos = SetIter->second;
+ SmallVector<EdgeInfo, 128> GlobWorklist;
+ for (auto &VI : llvm::make_early_inc_range(ValueInfos)) {
+ auto Candidates =
+ qualifyCalleeCandidates(Index, VI.getSummaryList(), ModName);
+
+ const GlobalValueSummary *GVS = nullptr;
+ FunctionImporter::ImportFailureReason LastReason =
+ FunctionImporter::ImportFailureReason::None;
+ for (const auto &Candidate : Candidates) {
+ LastReason = Candidate.first;
+ if (Candidate.first == FunctionImporter::ImportFailureReason::None) {
+ const bool Prevailing = IsPrevailing(VI.getGUID(), Candidate.second);
+ if (Prevailing || !GVS) {
+ if (!GVS && !Prevailing)
+ LLVM_DEBUG(dbgs()
+ << "[Workload] Considering " << VI.name() << " from "
+ << Candidate.second->modulePath() << " with linkage "
+ << Candidate.second->linkage()
+ << " although it's not prevailing, but it's the "
+ "first available candidate.\n");
+ GVS = Candidate.second;
+ if (Prevailing) {
+ LLVM_DEBUG(dbgs()
+ << "[Workload] Considering " << VI.name() << " from "
+ << GVS->modulePath() << " with linkage "
+ << GVS->linkage() << " because it's prevailing.\n");
+ break;
+ }
+ } else {
+ LLVM_DEBUG(dbgs() << "[Workload] Skipping " << VI.name() << " from "
+ << Candidate.second->modulePath()
+ << " with linkage " << Candidate.second->linkage()
+ << " because it's not prevailing\n");
+ }
+ }
+ }
+ if (!GVS) {
+ LLVM_DEBUG(dbgs() << "[Workload] Not importing " << VI.name()
+ << " because can't select Callee. Guid is: "
+ << Function::getGUID(VI.name())
+ << ". The reason was: " << getFailureName(LastReason)
+ << "\n");
+ continue;
+ }
+ const auto *CFS = cast<FunctionSummary>(GVS->getBaseObject());
+ auto ExportingModule = CFS->modulePath();
+ if (ExportingModule == ModName) {
+ LLVM_DEBUG(dbgs() << "[Workload] Not importing " << VI.name()
+ << " because its defining module is the same as the "
+ "current module\n");
+ continue;
+ }
+ if (!shouldImport(DefinedGVSummaries, VI.getGUID(), CFS)) {
+ LLVM_DEBUG(dbgs() << "[Workload] Not importing " << VI.name()
+ << " because we have a local copy.\n");
+ continue;
+ }
+
+ LLVM_DEBUG(dbgs() << "[Workload][Including]" << VI.name() << " from "
+ << ExportingModule << " : "
+ << Function::getGUID(VI.name()) << "\n");
+ ImportList[ExportingModule].insert(VI.getGUID());
+ GVI.onImportingSummary(*GVS);
+ if (ExportLists)
+ (*ExportLists)[ExportingModule].insert(VI);
+ }
+ LLVM_DEBUG(dbgs() << "[Workload] Done\n");
+ }
+
+ bool shouldImport(const GVSummaryMapTy &DefinedGVSummaries,
+ Function::GUID Guid, const GlobalValueSummary *Candidate) {
+ auto DefinedSummary = DefinedGVSummaries.find(Guid);
+ if (DefinedSummary == DefinedGVSummaries.end())
+ return true;
+
+ // See shouldImportGlobal for the justificaton of the isInterposableLinkage.
+ if (!IsPrevailing(Guid, DefinedSummary->second) &&
+ GlobalValue::isInterposableLinkage(DefinedSummary->second->linkage()) &&
+ IsPrevailing(Guid, Candidate)) {
+ LLVM_DEBUG(dbgs() << "[Workload] " << Guid
+ << ": local non-prevailing in module. Importing from "
+ << Candidate->modulePath() << "\n");
+ return true;
+ }
+ LLVM_DEBUG(dbgs() << "[Workload] " << Guid
+ << ": ignored! Target already in destination module.\n");
+ return false;
+ }
+
+public:
+ WorkloadImportsManager(
+ function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+ IsPrevailing,
+ const ModuleSummaryIndex &Index,
+ DenseMap<StringRef, FunctionImporter::ExportSetTy> *ExportLists)
+ : ModuleImportsManager(IsPrevailing, Index, ExportLists) {
+ StringMap<ValueInfo> CtxGuidToValueInfo;
+ for (auto &I : Index) {
+ ValueInfo VI(Index.haveGVs(), &I);
+ CtxGuidToValueInfo[VI.name()] = VI;
+ }
+ std::error_code EC;
+ auto BufferOrErr = MemoryBuffer::getFileOrSTDIN(WorkloadDefinitions);
+ if (std::error_code EC = BufferOrErr.getError()) {
+ report_fatal_error("Failed to open context file");
+ return;
+ }
+ auto Buffer = std::move(BufferOrErr.get());
+ std::map<std::string, std::vector<std::string>> WorkloadDefs;
+ json::Path::Root NullRoot;
+ auto Parsed = json::parse(Buffer->getBuffer());
+ if (!Parsed)
+ report_fatal_error(Parsed.takeError());
+ if (!json::fromJSON(*Parsed, WorkloadDefs, NullRoot))
+ report_fatal_error("Invalid thinlto contextual profile format.");
+ for (const auto &Workload : WorkloadDefs) {
+ const auto &Root = Workload.first;
+ LLVM_DEBUG(dbgs() << "[Workload] Root: " << Root << "\n");
+ const auto &AllCallees = Workload.second;
+ auto RootIt = CtxGuidToValueInfo.find(Root);
+ if (RootIt == CtxGuidToValueInfo.end()) {
+ LLVM_DEBUG(dbgs() << "[Workload] Root " << Root
+ << " not found in this linkage unit.\n");
+ continue;
+ }
+ auto RootVI = RootIt->second;
+ if (RootVI.getSummaryList().size() != 1) {
+ LLVM_DEBUG(dbgs() << "[Workload] Root " << Root
+ << " should have exactly one summary, but has "
+ << RootVI.getSummaryList().size() << ". Skipping.\n");
+ continue;
+ }
+ StringRef RootDefiningModule =
+ RootVI.getSummaryList().front()->modulePath();
+ LLVM_DEBUG(dbgs() << "[Workload] Root defining module for " << Root
+ << " is : " << RootDefiningModule << "\n");
+ auto &Set = Workloads[RootDefiningModule];
+ for (const auto &Callee : AllCallees) {
+ LLVM_DEBUG(dbgs() << "[Workload] " << Callee << "\n");
+ auto ElemIt = CtxGuidToValueInfo.find(Callee);
+ if (ElemIt == CtxGuidToValueInfo.end()) {
+ LLVM_DEBUG(dbgs() << "[Workload] " << Callee << " not found\n");
+ continue;
+ }
+ Set.insert(ElemIt->second);
+ }
+ LLVM_DEBUG(dbgs() << "[Workload] Root: " << Root << " we have "
+ << Set.size() << " distinct callees.\n");
+ LLVM_DEBUG( //
+ for (const auto &VI
+ : Set) {
+ dbgs() << "[Workload] Root: " << Root
+ << " Would include: " << VI.getGUID() << "\n";
+ });
+ }
+ }
+};
+
+std::unique_ptr<ModuleImportsManager> ModuleImportsManager::create(
+ function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
+ IsPrevailing,
+ const ModuleSummaryIndex &Index,
+ DenseMap<StringRef, FunctionImporter::ExportSetTy> *ExportLists) {
+ if (WorkloadDefinitions.empty()) {
+ LLVM_DEBUG(dbgs() << "[Workload] Using the regular imports manager.\n");
+ return std::unique_ptr<ModuleImportsManager>(
+ new ModuleImportsManager(IsPrevailing, Index, ExportLists));
+ }
+ LLVM_DEBUG(dbgs() << "[Workload] Using the contextual imports manager.\n");
+ return std::make_unique<WorkloadImportsManager>(IsPrevailing, Index,
+ ExportLists);
+}
+
static const char *
getFailureName(FunctionImporter::ImportFailureReason Reason) {
switch (Reason) {
@@ -732,14 +945,14 @@ void llvm::ComputeCrossModuleImport(
isPrevailing,
DenseMap<StringRef, FunctionImporter::ImportMapTy> &ImportLists,
DenseMap<StringRef, FunctionImporter::ExportSetTy> &ExportLists) {
- ModuleImportsManager MIS(isPrevailing, Index, &ExportLists);
+ auto MIS = ModuleImportsManager::create(isPrevailing, Index, &ExportLists);
// For each module that has function defined, compute the import/export lists.
for (const auto &DefinedGVSummaries : ModuleToDefinedGVSummaries) {
auto &ImportList = ImportLists[DefinedGVSummaries.first];
LLVM_DEBUG(dbgs() << "Computing import for Module '"
<< DefinedGVSummaries.first << "'\n");
- MIS.computeImportForModule(DefinedGVSummaries.second,
- DefinedGVSummaries.first, ImportList);
+ MIS->computeImportForModule(DefinedGVSummaries.second,
+ DefinedGVSummaries.first, ImportList);
}
// When computing imports we only added the variables and functions being
@@ -855,8 +1068,8 @@ static void ComputeCrossModuleImportForModuleForTest(
// Compute the import list for this module.
LLVM_DEBUG(dbgs() << "Computing import for Module '" << ModulePath << "'\n");
- ModuleImportsManager MIS(isPrevailing, Index);
- MIS.computeImportForModule(FunctionSummaryMap, ModulePath, ImportList);
+ auto MIS = ModuleImportsManager::create(isPrevailing, Index);
+ MIS->computeImportForModule(FunctionSummaryMap, ModulePath, ImportList);
#ifndef NDEBUG
dumpImportListForModule(Index, ModulePath, ImportList);
diff --git a/llvm/test/ThinLTO/X86/Inputs/workload1.ll b/llvm/test/ThinLTO/X86/Inputs/workload1.ll
new file mode 100644
index 0000000000000..0037232f9fec3
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/Inputs/workload1.ll
@@ -0,0 +1,25 @@
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+declare void @m1_variant()
+
+define dso_local void @m1_f1() {
+ call void @m1_f2()
+ call void @noninterposable_f()
+ ret void
+}
+
+define internal void @m1_f2() {
+ call void @interposable_f()
+ ret void
+}
+
+define linkonce void @interposable_f() {
+ call void @m1_variant()
+ ret void
+}
+
+define linkonce_odr void @noninterposable_f() {
+ call void @m1_variant()
+ ret void
+}
\ No newline at end of file
diff --git a/llvm/test/ThinLTO/X86/Inputs/workload2.ll b/llvm/test/ThinLTO/X86/Inputs/workload2.ll
new file mode 100644
index 0000000000000..cbb51b6b11ad9
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/Inputs/workload2.ll
@@ -0,0 +1,22 @@
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+declare void @m2_variant()
+
+define dso_local void @m2_f1() {
+ call void @interposable_f()
+ call void @noninterposable_f()
+ ret void
+}
+
+@m2_f1_alias = alias void (...), ptr @m2_f1
+
+define linkonce_odr void @interposable_f() {
+ call void @m2_variant()
+ ret void
+}
+
+define linkonce_odr void @noninterposable_f() {
+ call void @m2_variant()
+ ret void
+}
\ No newline at end of file
diff --git a/llvm/test/ThinLTO/X86/Inputs/workload3.ll b/llvm/test/ThinLTO/X86/Inputs/workload3.ll
new file mode 100644
index 0000000000000..01b79a4f334fd
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/Inputs/workload3.ll
@@ -0,0 +1,9 @@
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+declare void @m1_f1()
+
+define dso_local void @m3_f1() {
+ call void @m1_f1()
+ ret void
+}
diff --git a/llvm/test/ThinLTO/X86/workload.ll b/llvm/test/ThinLTO/X86/workload.ll
new file mode 100644
index 0000000000000..d98ecf630de26
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/workload.ll
@@ -0,0 +1,63 @@
+; RUN: mkdir -p %t_baseline
+; RUN: echo '{"m1_f1":["m2_f1", "m2_f1_alias", "interposable_f", "noninterposable_f"], \
+; RUN: "m2_f1":["m1_f1", "m1_f2"]}' > %t/workload_defs.json
+; RUN: opt -module-summary %S/Inputs/workload1.ll -o %t_baseline/workload1.bc
+; RUN: opt -module-summary %S/Inputs/workload2.ll -o %t_baseline/workload2.bc
+; RUN: opt -module-summary %S/Inputs/workload3.ll -o %t_baseline/workload3.bc
+;
+; Normal run. The first module shouldn't get m2_f1
+;
+; RUN: llvm-lto2 run %t_baseline/workload1.bc %t_baseline/workload2.bc %t_baseline/workload3.bc \
+; RUN: -o %t_baseline/result.o -save-temps \
+; RUN: -r %t_baseline/workload1.bc,m1_f1,plx \
+; RUN: -r %t_baseline/workload1.bc,interposable_f \
+; RUN: -r %t_baseline/workload1.bc,noninterposable_f \
+; RUN: -r %t_baseline/workload1.bc,m1_variant \
+; RUN: -r %t_baseline/workload2.bc,m2_f1,plx \
+; RUN: -r %t_baseline/workload2.bc,m2_f1_alias \
+; RUN: -r %t_baseline/workload2.bc,interposable_f,p \
+; RUN: -r %t_baseline/workload2.bc,noninterposable_f,p \
+; RUN: -r %t_baseline/workload2.bc,m2_variant \
+; RUN: -r %t_baseline/workload3.bc,m1_f1 \
+; RUN: -r %t_baseline/workload3.bc,m3_f1,plx
+; RUN: llvm-dis %t_baseline/result.o.1.3.import.bc -o - | FileCheck %s --check-prefix=NOPROF
+;
+; NOPROF-NOT: m2_f1
+;
+; The run with workload definitions - same other options.
+;
+; RUN: llvm-lto2 run %t/workload1.bc %t/workload2.bc %t/workload3.bc \
+; RUN: -thinlto-workload-def=%t/workload_defs.json -o %t/result.o -save-temps \
+; RUN: -r %t/workload1.bc,m1_f1,plx \
+; RUN: -r %t/workload1.bc,interposable_f \
+; RUN: -r %t/workload1.bc,noninterposable_f \
+; RUN: -r %t/workload1.bc,m1_variant \
+; RUN: -r %t/workload2.bc,m2_f1,plx \
+; RUN: -r %t/workload2.bc,m2_f1_alias \
+; RUN: -r %t/workload2.bc,interposable_f,p \
+; RUN: -r %t/workload2.bc,noninterposable_f,p \
+; RUN: -r %t/workload2.bc,m2_variant \
+; RUN: -r %t/workload3.bc,m1_f1 \
+; RUN: -r %t/workload3.bc,m3_f1,plx
+; RUN: llvm-dis %t/result.o.1.3.import.bc -o - | FileCheck %s --check-prefix=FIRST
+; RUN: llvm-dis %t/result.o.2.3.import.bc -o - | FileCheck %s --check-prefix=SECOND
+; RUN: llvm-dis %t/result.o.3.3.import.bc -o - | FileCheck %s --check-prefix=THIRD
+;
+; The third module is bitwse-identical to the "normal" run, as the
+; RUN: diff %t_baseline/result.o.3.3.import.bc %t/result.o.3.3.import.bc
+;
+; This time, we expect m1 to have m2_f1 and the m2 variant of interposable_f,
+; while keeping its variant of noninterposable_f
+;
+; FIRST-LABEL: @m1_f1
+; FIRST-LABEL: @m1_f2
+; FIRST-LABEL: define available_externally void @noninterposable_f
+; FIRST-NEXT: call void @m1_variant
+; FIRST-LABEL: @m2_f1
+; FIRST-LABEL: define available_externally void @interposable_f
+; FIRST-NEXT: call void @m2_variant
+; FIRST-NOT: @m2_f1_alias
+; SECOND-LABEL: @m2_f1
+; SECOND-LABEL: @m1_f1
+; SECOND-LABEL: @m1_f2
+; THIRD-LABEL: define available_externally void @m1_f1
\ No newline at end of file
|
llvm/test/ThinLTO/X86/workload.ll
Outdated
| ; FIRST-LABEL: @m2_f1 | ||
| ; FIRST-LABEL: define available_externally void @interposable_f | ||
| ; FIRST-NEXT: call void @m2_variant | ||
| ; FIRST-NOT: @m2_f1_alias |
There was a problem hiding this comment.
Why isn't m2_f1_alias imported, since it was specified in the import list of m1? I guess that is due to the way the importer handles imports (?) - it would be good to have a comment as to why.
There was a problem hiding this comment.
It should be actually. (summarizing offline discussion): We need to assume aliases and aliasees are given in the import list because the source of the workload def may be a tool working off sections in the binary, and there's no information as to which name is alias or aliasee - all we know is that multiple symbols map to the same address. That aside, we will then import what's given in the list. Today, that will mean that if a function and its alias are imported, the alias will be instantiated as a clone of the original. That's likely rare; anyway, it can be addressed subsequently (i.e. have a mode that, instead of cloning, replaces callsites)
| ; FIRST-LABEL: define available_externally void @noninterposable_f | ||
| ; FIRST-NEXT: call void @m2_variant | ||
| ; | ||
| ; FIRST-LABEL: define available_externally void @m2_f1_alias | ||
| ; SECOND-LABEL: @m2_f1 |
There was a problem hiding this comment.
Add comment for what we expect in the SECOND case. Ditto below for THIRD. Analogous to your "This time, we expect m1 ..." comment for FIRST above.
| @@ -1,67 +1,153 @@ | |||
| ; Set up | |||
There was a problem hiding this comment.
Nit: add brief comment at the top about what this is testing (e.g. the workload based importing via -thinlto-workload-def.
Also suggest moving the current m1.ll, m2.ll, and m3.ll code to the end so that all the test RUN lines and checks are at the top.
| @@ -588,7 +568,11 @@ class WorkloadImportsManager : public ModuleImportsManager { | |||
| std::map<std::string, std::vector<std::string>> WorkloadDefs; | |||
| json::Path::Root NullRoot; | |||
| // The JSON is supposed to contain a dictionary matching the type of | |||
| // WorkloadDefs. | |||
| // WorkloadDefs. For example: | |||
There was a problem hiding this comment.
Suggest moving this example to the option definition.
You can test this locally with the following command:git-clang-format --diff 7ec4f6094e54911794c142b5d88496a220d807d6 d0174578d0af2a8ee1d6719e276bd60530cff392 -- llvm/lib/Transforms/IPO/FunctionImport.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 49b3f2b085..6700e2429e 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -479,7 +479,7 @@ class WorkloadImportsManager : public ModuleImportsManager {
<< " ImportFailureReason: "
<< getFailureName(Candidate.first) << "\n");
return Candidate.first ==
- FunctionImporter::ImportFailureReason::None;
+ FunctionImporter::ImportFailureReason::None;
}),
[](const auto &Candidate) { return Candidate.second; });
if (PotentialCandidates.empty()) {
|
| return IsPrevailing(VI.getGUID(), Candidate); | ||
| }); | ||
| if (PrevailingCandidates.empty()) { | ||
| if (!llvm::hasSingleElement(PotentialCandidates)) |
There was a problem hiding this comment.
We could in theory have multiple (weak) copies of a symbol when there is no prevailing candidate, if say the prevailing copy was in a native object being linked in. However, we should in theory be marking all of these non-prevailing IR copies dead in that case, in which case they won't be candidates, so I don't think we can get here. Maybe verify that the non-prevailing copy that we select below has local linkage?
There was a problem hiding this comment.
added your comment, too. also might as well check it's live.
teresajohnson
approved these changes
Dec 14, 2023
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.
An example of a "workload definition" would be "the transitive closure of functions actually called to satisfy a RPC request", i.e. a (typically significantly) smaller subset of the transitive closure (static + possible indirect call targets) of callees. This means this workload definition is a type of flat dynamic profile.
Producing one is not in scope - it can be produced offline from traces, or from sample-based profiles, etc.
This patch adds awareness to ThinLTO of such a concept. A workload is defined as a root and a list of functions. All function references are by-name (more readable than GUIDs). In the case of aliases, the expectation is the list contains all the alternative names.
The workload definitions are presented to the linker as a json file, containing a dictionary. The keys are the roots, the values are the list of functions.
The import list for a module defining a root will be the functions listed for it in the profile.
Using names this way assumes unique names for internal functions, i.e. clang's
-funique-internal-linkage-names.Note that the behavior affects the entire module where a root is defined (i.e. different workloads best be defined in different modules), and does not affect modules that don't define roots.