[StaleProfileMatching] Use only profile anchor size for similarity calculation #126783
Conversation
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-llvm-transforms Author: Lei Wang (wlei-llvm) ChangesWe observed that the number of IR anchors is usually greater than the number of profile anchors, because IR anchors can be optimized away later and llvm-profgen might not generate profiles for cold callsites. This can cause missing function matches. I’m changing the similarity calculation to use only the profile anchor size. In another point of view, It also makes sense to reuse as many profile anchors as possible regardless of the new functions in the IR. Full diff: https://github.com/llvm/llvm-project/pull/126783.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp b/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp
index 313a50477a314..64fcb49d44439 100644
--- a/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp
@@ -23,7 +23,7 @@ using namespace sampleprof;
#define DEBUG_TYPE "sample-profile-matcher"
static cl::opt<unsigned> FuncProfileSimilarityThreshold(
- "func-profile-similarity-threshold", cl::Hidden, cl::init(80),
+ "func-profile-similarity-threshold", cl::Hidden, cl::init(70),
cl::desc("Consider a profile matches a function if the similarity of their "
"callee sequences is above the specified percentile."));
@@ -790,9 +790,8 @@ bool SampleProfileMatcher::functionMatchesProfileHelper(
longestCommonSequence(FilteredIRAnchorsList, FilteredProfileAnchorList,
false /* Match unused functions */);
- Similarity =
- static_cast<float>(MatchedAnchors.size()) * 2 /
- (FilteredIRAnchorsList.size() + FilteredProfileAnchorList.size());
+ Similarity = static_cast<float>(MatchedAnchors.size()) /
+ FilteredProfileAnchorList.size();
LLVM_DEBUG(dbgs() << "The similarity between " << IRFunc.getName()
<< "(IR) and " << ProfFunc << "(profile) is "
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-stale-profile-renaming.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-stale-profile-renaming.ll
index a549812f46ef6..8b40d6bf49f80 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-stale-profile-renaming.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-stale-profile-renaming.ll
@@ -8,7 +8,7 @@
; CHECK: Function new_foo is not in profile or profile symbol list.
; CHECK: Run stale profile matching for main
-; CHECK: The similarity between new_foo(IR) and foo(profile) is 0.86
+; CHECK: The similarity between new_foo(IR) and foo(profile) is 0.75
; CHECK: Function:new_foo matches profile:foo
; CHECK: Run stale profile matching for cold_func
; CHECK: The checksums for new_block_only(IR) and block_only(Profile) match.
|
| @@ -23,7 +23,7 @@ using namespace sampleprof; | |||
| #define DEBUG_TYPE "sample-profile-matcher" | |||
|
|
|||
| static cl::opt<unsigned> FuncProfileSimilarityThreshold( | |||
| "func-profile-similarity-threshold", cl::Hidden, cl::init(80), | |||
| "func-profile-similarity-threshold", cl::Hidden, cl::init(70), | |||
There was a problem hiding this comment.
any explanation for the change in threshold? in addition to the change in how it's calculated
There was a problem hiding this comment.
This is just from my observation, may not be accurate. I observed that there were many false negatives(we missed the good match). Recalling that it's under the LCS frame, the matching scope is narrowed, it's very likely to have a good match. So I feel overall threshold is a bit too high, so try to lower it a bit.
There was a problem hiding this comment.
Is 10% drop meaningful, is 70% final? If so, it's okay to include it here, otherwise the tuning can be done separately.
There was a problem hiding this comment.
Is 10% drop meaningful, is 70% final? If so, it's okay to include it here, otherwise the tuning can be done separately.
I see, sounds good to tune separately.
…lculation (llvm#126783) We observed that the number of IR anchors is usually greater than the number of profile anchors, because IR anchors can be optimized away later and llvm-profgen might not generate profiles for cold callsites. This can cause missing function matches. I’m changing the similarity calculation to use only the profile anchor size. In another point of view, It also makes sense to reuse as many profile anchors as possible regardless of the new functions in the IR.
…lculation (llvm#126783) We observed that the number of IR anchors is usually greater than the number of profile anchors, because IR anchors can be optimized away later and llvm-profgen might not generate profiles for cold callsites. This can cause missing function matches. I’m changing the similarity calculation to use only the profile anchor size. In another point of view, It also makes sense to reuse as many profile anchors as possible regardless of the new functions in the IR.
We observed that the number of IR anchors is usually greater than the number of profile anchors, because IR anchors can be optimized away later and llvm-profgen might not generate profiles for cold callsites. This can cause missing function matches. I’m changing the similarity calculation to use only the profile anchor size. In another point of view, It also makes sense to reuse as many profile anchors as possible regardless of the new functions in the IR.