[llvm] [CSSPGO] Error out if the checksum mismatch is high (PR #84097)
Lei Wang via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 20 20:29:07 PDT 2024
https://github.com/wlei-llvm updated https://github.com/llvm/llvm-project/pull/84097
>From b7ede70a4f56e0bea4ed37992a6fcf9aa971ddef Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Thu, 29 Feb 2024 18:04:37 -0800
Subject: [PATCH 1/6] [CSSPGO] Error out if the checksum mismatch is high
---
llvm/lib/Transforms/IPO/SampleProfile.cpp | 81 +++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
index ffc26f1a0a972d..9ec224bb32a551 100644
--- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
@@ -234,6 +234,24 @@ static cl::opt<unsigned> ProfileICPRelativeHotnessSkip(
cl::desc(
"Skip relative hotness check for ICP up to given number of targets."));
+static cl::opt<unsigned> ChecksumMismatchFuncHotBlockSkip(
+ "checksum-mismatch-func-hot-block-skip", cl::Hidden, cl::init(100),
+ cl::desc("For checksum-mismatch error check, skip checking the function "
+ "whose num of hot(on average) blocks is smaller than the "
+ "given number."));
+
+static cl::opt<unsigned> ChecksumMismatchNumFuncSkip(
+ "checksum-mismatch-num-func-skip", cl::Hidden, cl::init(50),
+ cl::desc("For checksum-mismatch error check, skip the check if the total "
+ "number of selected function is smaller than the given number."));
+
+static cl::opt<unsigned> ChecksumMismatchErrorThreshold(
+ "checksum-mismatch-error-threshold", cl::Hidden, cl::init(80),
+ cl::desc(
+ "For checksum-mismatch error check, error out if the percentage of "
+ "function mismatched-checksum is higher than the given percentage "
+ "threshold"));
+
static cl::opt<bool> CallsitePrioritizedInline(
"sample-profile-prioritized-inline", cl::Hidden,
@@ -630,6 +648,8 @@ class SampleProfileLoader final : public SampleProfileLoaderBaseImpl<Function> {
std::vector<Function *> buildFunctionOrder(Module &M, LazyCallGraph &CG);
std::unique_ptr<ProfiledCallGraph> buildProfiledCallGraph(Module &M);
void generateMDProfMetadata(Function &F);
+ bool errorIfHighChecksumMismatch(Module &M, ProfileSummaryInfo *PSI,
+ const SampleProfileMap &Profiles);
/// Map from function name to Function *. Used to find the function from
/// the function name. If the function name contains suffix, additional
@@ -2184,6 +2204,61 @@ bool SampleProfileLoader::doInitialization(Module &M,
return true;
}
+// Note that this is a module-level check. Even if one module is errored out,
+// the entire build will be errored out. However, the user could make big
+// changes to functions in single module but those changes might not be
+// performance significant to the whole binary. Therefore, we use a conservative
+// approach to make sure we only error out if it globally impacts the binary
+// performance. To achieve this, we use heuristics to select a reasonable
+// big set of functions that are supposed to be globally performance
+// significant, only compute and check the mismatch within those functions. The
+// function selection is based on two criteria: 1) The function is "hot" enough,
+// which is tuned by a hotness-based flag(ChecksumMismatchFuncHotBlockSkip). 2)
+// The num of function is large enough which is tuned by the
+// ChecksumMismatchNumFuncSkip flag.
+bool SampleProfileLoader::errorIfHighChecksumMismatch(
+ Module &M, ProfileSummaryInfo *PSI, const SampleProfileMap &Profiles) {
+ assert(FunctionSamples::ProfileIsProbeBased &&
+ "Only support for probe-based profile");
+ uint64_t TotalSelectedFunc = 0;
+ uint64_t NumMismatchedFunc = 0;
+ for (const auto &I : Profiles) {
+ const auto &FS = I.second;
+ const auto *FuncDesc = ProbeManager->getDesc(FS.getGUID());
+ if (!FuncDesc)
+ continue;
+
+ // We want to select a set of functions that are globally performance
+ // significant, in other words, if those functions profiles are
+ // checksum-mismatched and dropped, the whole binary will likely be
+ // impacted, so here we use a hotness-based threshold to control the
+ // selection.
+ if (FS.getTotalSamples() <
+ ChecksumMismatchFuncHotBlockSkip * PSI->getOrCompHotCountThreshold())
+ continue;
+
+ TotalSelectedFunc++;
+ if (ProbeManager->profileIsHashMismatched(*FuncDesc, FS))
+ NumMismatchedFunc++;
+ }
+ // Make sure that the num of selected function is not too small to distinguish
+ // from the user's benign changes.
+ if (TotalSelectedFunc < ChecksumMismatchNumFuncSkip)
+ return false;
+
+ // Finally check the mismatch percentage against the threshold.
+ if (NumMismatchedFunc * 100 >=
+ TotalSelectedFunc * ChecksumMismatchErrorThreshold) {
+ auto &Ctx = M.getContext();
+ const char *Msg =
+ "The FDO profile is too old and will cause big performance regression, "
+ "please drop the profile and collect a new one.";
+ Ctx.diagnose(DiagnosticInfoSampleProfile(M.getModuleIdentifier(), Msg));
+ return true;
+ }
+ return false;
+}
+
void SampleProfileMatcher::findIRAnchors(
const Function &F, std::map<LineLocation, StringRef> &IRAnchors) {
// For inlined code, recover the original callsite and callee by finding the
@@ -2715,6 +2790,12 @@ bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM,
ProfileSummary::PSK_Sample);
PSI->refresh();
}
+
+ // Error out if the profile checksum mismatch is too high.
+ if (FunctionSamples::ProfileIsProbeBased &&
+ errorIfHighChecksumMismatch(M, PSI, Reader->getProfiles()))
+ return false;
+
// Compute the total number of samples collected in this profile.
for (const auto &I : Reader->getProfiles())
TotalCollectedSamples += I.second.getTotalSamples();
>From 2f5de6099a5b70ac9d1df1dd72d4406acfd2aae9 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Tue, 5 Mar 2024 16:06:53 -0800
Subject: [PATCH 2/6] add test
---
.../SampleProfile/pseudo-probe-profile-mismatch-error.ll | 5 +++++
1 file changed, 5 insertions(+)
create mode 100644 llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
new file mode 100644
index 00000000000000..84a2a0ff2eb6ad
--- /dev/null
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
@@ -0,0 +1,5 @@
+; REQUIRES: x86_64-linux
+; RUN: opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -checksum-mismatch-func-hot-block-skip=0 -checksum-mismatch-num-func-skip=1 -checksum-mismatch-error-threshold=1 -S 2>%t -o %t.ll
+; RUN: FileCheck %s --input-file %t
+
+; CHECK: error: [[*]]: The FDO profile is too old and will cause big performance regression, please drop the profile and collect a new one.
>From 5a77cbeb072af63d9ff8979af42ae63a7e2443c1 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Fri, 15 Mar 2024 13:56:28 -0700
Subject: [PATCH 3/6] addressing comments
---
llvm/lib/Transforms/IPO/SampleProfile.cpp | 38 +++++++++----------
.../pseudo-probe-profile-mismatch-error.ll | 5 +--
2 files changed, 20 insertions(+), 23 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
index 9ec224bb32a551..a45d1e9de5816b 100644
--- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
@@ -240,17 +240,15 @@ static cl::opt<unsigned> ChecksumMismatchFuncHotBlockSkip(
"whose num of hot(on average) blocks is smaller than the "
"given number."));
-static cl::opt<unsigned> ChecksumMismatchNumFuncSkip(
- "checksum-mismatch-num-func-skip", cl::Hidden, cl::init(50),
- cl::desc("For checksum-mismatch error check, skip the check if the total "
- "number of selected function is smaller than the given number."));
+static cl::opt<unsigned> MinfuncsForStalenessError(
+ "min-functions-for-staleness-error", cl::Hidden, cl::init(50),
+ cl::desc("Skip the check if the number of hot functions is smaller than "
+ "the given number."));
-static cl::opt<unsigned> ChecksumMismatchErrorThreshold(
- "checksum-mismatch-error-threshold", cl::Hidden, cl::init(80),
- cl::desc(
- "For checksum-mismatch error check, error out if the percentage of "
- "function mismatched-checksum is higher than the given percentage "
- "threshold"));
+static cl::opt<unsigned> PrecentMismatchForStalenessError(
+ "precent-mismatch-for-staleness-error", cl::Hidden, cl::init(80),
+ cl::desc("Reject the profile if the mismatch percent is higher than the "
+ "given number"));
static cl::opt<bool> CallsitePrioritizedInline(
"sample-profile-prioritized-inline", cl::Hidden,
@@ -648,7 +646,7 @@ class SampleProfileLoader final : public SampleProfileLoaderBaseImpl<Function> {
std::vector<Function *> buildFunctionOrder(Module &M, LazyCallGraph &CG);
std::unique_ptr<ProfiledCallGraph> buildProfiledCallGraph(Module &M);
void generateMDProfMetadata(Function &F);
- bool errorIfHighChecksumMismatch(Module &M, ProfileSummaryInfo *PSI,
+ bool rejectHighStalenessProfile(Module &M, ProfileSummaryInfo *PSI,
const SampleProfileMap &Profiles);
/// Map from function name to Function *. Used to find the function from
@@ -2215,12 +2213,12 @@ bool SampleProfileLoader::doInitialization(Module &M,
// function selection is based on two criteria: 1) The function is "hot" enough,
// which is tuned by a hotness-based flag(ChecksumMismatchFuncHotBlockSkip). 2)
// The num of function is large enough which is tuned by the
-// ChecksumMismatchNumFuncSkip flag.
-bool SampleProfileLoader::errorIfHighChecksumMismatch(
+// MinfuncsForStalenessError flag.
+bool SampleProfileLoader::rejectHighStalenessProfile(
Module &M, ProfileSummaryInfo *PSI, const SampleProfileMap &Profiles) {
assert(FunctionSamples::ProfileIsProbeBased &&
"Only support for probe-based profile");
- uint64_t TotalSelectedFunc = 0;
+ uint64_t TotalHotFunc = 0;
uint64_t NumMismatchedFunc = 0;
for (const auto &I : Profiles) {
const auto &FS = I.second;
@@ -2237,22 +2235,22 @@ bool SampleProfileLoader::errorIfHighChecksumMismatch(
ChecksumMismatchFuncHotBlockSkip * PSI->getOrCompHotCountThreshold())
continue;
- TotalSelectedFunc++;
+ TotalHotFunc++;
if (ProbeManager->profileIsHashMismatched(*FuncDesc, FS))
NumMismatchedFunc++;
}
// Make sure that the num of selected function is not too small to distinguish
// from the user's benign changes.
- if (TotalSelectedFunc < ChecksumMismatchNumFuncSkip)
+ if (TotalHotFunc < MinfuncsForStalenessError)
return false;
// Finally check the mismatch percentage against the threshold.
if (NumMismatchedFunc * 100 >=
- TotalSelectedFunc * ChecksumMismatchErrorThreshold) {
+ TotalHotFunc * PrecentMismatchForStalenessError) {
auto &Ctx = M.getContext();
const char *Msg =
- "The FDO profile is too old and will cause big performance regression, "
- "please drop the profile and collect a new one.";
+ "The input profile significantly mismatches current source code. "
+ "Please recollect profile to avoid performance regression.";
Ctx.diagnose(DiagnosticInfoSampleProfile(M.getModuleIdentifier(), Msg));
return true;
}
@@ -2793,7 +2791,7 @@ bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM,
// Error out if the profile checksum mismatch is too high.
if (FunctionSamples::ProfileIsProbeBased &&
- errorIfHighChecksumMismatch(M, PSI, Reader->getProfiles()))
+ rejectHighStalenessProfile(M, PSI, Reader->getProfiles()))
return false;
// Compute the total number of samples collected in this profile.
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
index 84a2a0ff2eb6ad..daf616c45c46e0 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
@@ -1,5 +1,4 @@
; REQUIRES: x86_64-linux
-; RUN: opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -checksum-mismatch-func-hot-block-skip=0 -checksum-mismatch-num-func-skip=1 -checksum-mismatch-error-threshold=1 -S 2>%t -o %t.ll
-; RUN: FileCheck %s --input-file %t
+; RUN: not opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -checksum-mismatch-func-hot-block-skip=0 -min-functions-for-staleness-error=1 -precent-mismatch-for-staleness-error=1 -S 2>&1 | FileCheck %s
-; CHECK: error: [[*]]: The FDO profile is too old and will cause big performance regression, please drop the profile and collect a new one.
+; CHECK: error: {{.*}}: The input profile significantly mismatches current source code. Please recollect profile to avoid performance regression.
>From 42502a34309d6e44420ee92834ba031bed2ac062 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Mon, 18 Mar 2024 22:42:29 -0700
Subject: [PATCH 4/6] Use isHotCountNthPercentile and total_samples to decide
if function is hot
---
llvm/lib/Transforms/IPO/SampleProfile.cpp | 17 ++++++++---------
.../pseudo-probe-profile-mismatch-error.ll | 2 +-
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
index a45d1e9de5816b..2d314998d334d9 100644
--- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
@@ -234,11 +234,10 @@ static cl::opt<unsigned> ProfileICPRelativeHotnessSkip(
cl::desc(
"Skip relative hotness check for ICP up to given number of targets."));
-static cl::opt<unsigned> ChecksumMismatchFuncHotBlockSkip(
- "checksum-mismatch-func-hot-block-skip", cl::Hidden, cl::init(100),
- cl::desc("For checksum-mismatch error check, skip checking the function "
- "whose num of hot(on average) blocks is smaller than the "
- "given number."));
+static cl::opt<unsigned> HotFuncCutoffForStalenessError(
+ "hot-func-cutoff-for-staleness-error", cl::Hidden, cl::init(999000),
+ cl::desc("Hot function cutoff for staleness error. It's percentile value "
+ "(multiplied by 10000), e.g. 995000 for 99.5 percentile."));
static cl::opt<unsigned> MinfuncsForStalenessError(
"min-functions-for-staleness-error", cl::Hidden, cl::init(50),
@@ -647,7 +646,7 @@ class SampleProfileLoader final : public SampleProfileLoaderBaseImpl<Function> {
std::unique_ptr<ProfiledCallGraph> buildProfiledCallGraph(Module &M);
void generateMDProfMetadata(Function &F);
bool rejectHighStalenessProfile(Module &M, ProfileSummaryInfo *PSI,
- const SampleProfileMap &Profiles);
+ const SampleProfileMap &Profiles);
/// Map from function name to Function *. Used to find the function from
/// the function name. If the function name contains suffix, additional
@@ -2211,7 +2210,7 @@ bool SampleProfileLoader::doInitialization(Module &M,
// big set of functions that are supposed to be globally performance
// significant, only compute and check the mismatch within those functions. The
// function selection is based on two criteria: 1) The function is "hot" enough,
-// which is tuned by a hotness-based flag(ChecksumMismatchFuncHotBlockSkip). 2)
+// which is tuned by a hotness-based flag(HotFuncCutoffForStalenessError). 2)
// The num of function is large enough which is tuned by the
// MinfuncsForStalenessError flag.
bool SampleProfileLoader::rejectHighStalenessProfile(
@@ -2231,8 +2230,8 @@ bool SampleProfileLoader::rejectHighStalenessProfile(
// checksum-mismatched and dropped, the whole binary will likely be
// impacted, so here we use a hotness-based threshold to control the
// selection.
- if (FS.getTotalSamples() <
- ChecksumMismatchFuncHotBlockSkip * PSI->getOrCompHotCountThreshold())
+ if (!PSI->isHotCountNthPercentile(HotFuncCutoffForStalenessError,
+ FS.getTotalSamples()))
continue;
TotalHotFunc++;
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
index daf616c45c46e0..c6264f74b87a46 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
@@ -1,4 +1,4 @@
; REQUIRES: x86_64-linux
-; RUN: not opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -checksum-mismatch-func-hot-block-skip=0 -min-functions-for-staleness-error=1 -precent-mismatch-for-staleness-error=1 -S 2>&1 | FileCheck %s
+; RUN: not opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -min-functions-for-staleness-error=1 -precent-mismatch-for-staleness-error=1 -S 2>&1 | FileCheck %s
; CHECK: error: {{.*}}: The input profile significantly mismatches current source code. Please recollect profile to avoid performance regression.
>From 47161ea2c58c95242f8d6b7d06a68a3201216201 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Tue, 19 Mar 2024 14:59:24 -0700
Subject: [PATCH 5/6] rephrase the flag description
---
llvm/lib/Transforms/IPO/SampleProfile.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
index 2d314998d334d9..d2d7465d7fc98d 100644
--- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
@@ -236,8 +236,9 @@ static cl::opt<unsigned> ProfileICPRelativeHotnessSkip(
static cl::opt<unsigned> HotFuncCutoffForStalenessError(
"hot-func-cutoff-for-staleness-error", cl::Hidden, cl::init(999000),
- cl::desc("Hot function cutoff for staleness error. It's percentile value "
- "(multiplied by 10000), e.g. 995000 for 99.5 percentile."));
+ cl::desc("A function is considered hot for staleness error check if its "
+ "total sample count is above the specified percentile(multiplied "
+ "by 10000, e.g. 995000 for 99.5 percentile)."));
static cl::opt<unsigned> MinfuncsForStalenessError(
"min-functions-for-staleness-error", cl::Hidden, cl::init(50),
>From 0b1fb2b029d84063b9c252ae9c5b3937903624e7 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Wed, 20 Mar 2024 20:21:44 -0700
Subject: [PATCH 6/6] addressing comments and set hot-func-cutoff 800000
---
llvm/lib/Transforms/IPO/SampleProfile.cpp | 5 ++---
.../SampleProfile/pseudo-probe-profile-mismatch-error.ll | 3 +++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
index d2d7465d7fc98d..816e91c85d2e65 100644
--- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
@@ -235,10 +235,9 @@ static cl::opt<unsigned> ProfileICPRelativeHotnessSkip(
"Skip relative hotness check for ICP up to given number of targets."));
static cl::opt<unsigned> HotFuncCutoffForStalenessError(
- "hot-func-cutoff-for-staleness-error", cl::Hidden, cl::init(999000),
+ "hot-func-cutoff-for-staleness-error", cl::Hidden, cl::init(800000),
cl::desc("A function is considered hot for staleness error check if its "
- "total sample count is above the specified percentile(multiplied "
- "by 10000, e.g. 995000 for 99.5 percentile)."));
+ "total sample count is above the specified percentile"));
static cl::opt<unsigned> MinfuncsForStalenessError(
"min-functions-for-staleness-error", cl::Hidden, cl::init(50),
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
index c6264f74b87a46..2bb8f677f40ca8 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile-mismatch-error.ll
@@ -1,4 +1,7 @@
; REQUIRES: x86_64-linux
; RUN: not opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -min-functions-for-staleness-error=1 -precent-mismatch-for-staleness-error=1 -S 2>&1 | FileCheck %s
+; RUN: opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -min-functions-for-staleness-error=3 -precent-mismatch-for-staleness-error=70 -S 2>&1
+; RUN: opt < %S/pseudo-probe-profile-mismatch.ll -passes=sample-profile -sample-profile-file=%S/Inputs/pseudo-probe-profile-mismatch.prof -min-functions-for-staleness-error=4 -precent-mismatch-for-staleness-error=1 -S 2>&1
+
; CHECK: error: {{.*}}: The input profile significantly mismatches current source code. Please recollect profile to avoid performance regression.
More information about the llvm-commits
mailing list