-
Notifications
You must be signed in to change notification settings - Fork 10.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SamplePGO] Handle FS discriminators in SampleProfileMatcher #90858
Conversation
Currently the code uses FunctionSamples::getCallSiteIdentifier which will sometimes incorrectly guess that FSAFDO discriminators are probe based and will convert them incorrectly. This change doesn't affect builds which don't use FSAFDO, it only fixes sample profile matching with FS discriminators. The test for this is manually updated to use discriminator value 15, which is a perfectly valid base discriminator in the FS world, but satisfies `isPseudoProbeDiscriminator`, so `getBaseDiscriminatorFromDiscriminator` will incorrectly extract the probe index from it. Note: this change only affects how the base discriminators will be extracted when doing stale profile matching in the IR-level sample profile loader. It doesn't add stale profile matching to the MIR-level FS profile loader pass.
@llvm/pr-subscribers-llvm-transforms Author: Krzysztof Pszeniczny (amharc) ChangesCurrently the code uses FunctionSamples::getCallSiteIdentifier which will sometimes incorrectly guess that FSAFDO discriminators are probe based and will convert them incorrectly. This change doesn't affect builds which don't use FSAFDO, it only fixes sample profile matching with FS discriminators. The test for this is manually updated to use discriminator value 15, which is a perfectly valid base discriminator in the FS world, but satisfies Note: this change only affects how the base discriminators will be extracted when doing stale profile matching in the IR-level sample profile loader. It doesn't add stale profile matching to the MIR-level FS profile loader pass. Full diff: https://github.com/llvm/llvm-project/pull/90858.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp b/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp
index 1ca89e0091daff..142660bcc58e34 100644
--- a/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfileMatcher.cpp
@@ -37,7 +37,8 @@ void SampleProfileMatcher::findIRAnchors(
DIL = DIL->getInlinedAt();
} while (DIL->getInlinedAt());
- LineLocation Callsite = FunctionSamples::getCallSiteIdentifier(DIL);
+ LineLocation Callsite = FunctionSamples::getCallSiteIdentifier(
+ DIL, FunctionSamples::ProfileIsFS);
StringRef CalleeName = PrevDIL->getSubprogramLinkageName();
return std::make_pair(Callsite, CalleeName);
};
@@ -82,7 +83,8 @@ void SampleProfileMatcher::findIRAnchors(
if (DIL->getInlinedAt()) {
IRAnchors.emplace(FindTopLevelInlinedCallsite(DIL));
} else {
- LineLocation Callsite = FunctionSamples::getCallSiteIdentifier(DIL);
+ LineLocation Callsite = FunctionSamples::getCallSiteIdentifier(
+ DIL, FunctionSamples::ProfileIsFS);
StringRef CalleeName = GetCanonicalCalleeName(dyn_cast<CallBase>(&I));
IRAnchors.emplace(Callsite, CalleeName);
}
diff --git a/llvm/test/Transforms/SampleProfile/Inputs/non-probe-stale-profile-matching.prof b/llvm/test/Transforms/SampleProfile/Inputs/non-probe-stale-profile-matching.prof
index 8e988515be8ed5..418f2c4af26460 100644
--- a/llvm/test/Transforms/SampleProfile/Inputs/non-probe-stale-profile-matching.prof
+++ b/llvm/test/Transforms/SampleProfile/Inputs/non-probe-stale-profile-matching.prof
@@ -10,12 +10,12 @@ main:9229397:0
7: 0
2: foo:1479916
1: 47663
- 1.1: 46683 bar:43238
+ 1.15: 46683 bar:43238
2: 4519 bar:4932
3: 48723
4: foo:1505537
1: 48604
- 1.1: 46965 bar:44479
+ 1.15: 46965 bar:44479
2: 4613 bar:4967
3: 49087
bar:2333388:196222
diff --git a/llvm/test/Transforms/SampleProfile/non-probe-stale-profile-matching.ll b/llvm/test/Transforms/SampleProfile/non-probe-stale-profile-matching.ll
index eb69c18add01b8..5394a00ced86ad 100644
--- a/llvm/test/Transforms/SampleProfile/non-probe-stale-profile-matching.ll
+++ b/llvm/test/Transforms/SampleProfile/non-probe-stale-profile-matching.ll
@@ -1,6 +1,6 @@
; REQUIRES: x86_64-linux
; REQUIRES: asserts
-; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/non-probe-stale-profile-matching.prof --salvage-stale-profile -S --debug-only=sample-profile,sample-profile-matcher,sample-profile-impl 2>&1 | FileCheck %s
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/non-probe-stale-profile-matching.prof --salvage-stale-profile -S --debug-only=sample-profile,sample-profile-matcher,sample-profile-impl -profile-isfs 2>&1 | FileCheck %s
; The profiled source code:
@@ -51,7 +51,7 @@
; CHECK: Run stale profile matching for bar
; CHECK: Run stale profile matching for foo
-; CHECK: Callsite with callee:bar is matched from 1.1 to 1.1
+; CHECK: Callsite with callee:bar is matched from 1.15 to 1.15
; CHECK: Callsite with callee:bar is matched from 2 to 2
; CHECK: Run stale profile matching for main
@@ -183,7 +183,7 @@ attributes #3 = { mustprogress nocallback nofree nosync nounwind willreturn memo
!15 = !DILocation(line: 7, column: 9, scope: !14)
!16 = !DILocation(line: 7, column: 7, scope: !14)
!17 = !DILocation(line: 7, column: 23, scope: !18)
-!18 = !DILexicalBlockFile(scope: !14, file: !10, discriminator: 2)
+!18 = !DILexicalBlockFile(scope: !14, file: !10, discriminator: 15)
!19 = !DILocation(line: 7, column: 15, scope: !18)
!20 = !DILocation(line: 8, column: 21, scope: !14)
!21 = !DILocation(line: 8, column: 15, scope: !14)
@@ -201,7 +201,7 @@ attributes #3 = { mustprogress nocallback nofree nosync nounwind willreturn memo
!33 = !DILocation(line: 14, column: 8, scope: !25)
!34 = !DILocation(line: 14, scope: !25)
!35 = !DILocation(line: 14, column: 21, scope: !36)
-!36 = !DILexicalBlockFile(scope: !25, file: !10, discriminator: 2)
+!36 = !DILexicalBlockFile(scope: !25, file: !10, discriminator: 15)
!37 = !DILocation(line: 14, column: 3, scope: !36)
!38 = !DILocation(line: 14, column: 3, scope: !39)
!39 = !DILexicalBlockFile(scope: !25, file: !10, discriminator: 4)
|
Tagging @wlei-llvm @xur-llvm @WenleiHe (I don't have commit access, so I can't directly assign reviewers). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
good catch! thanks for the fix. |
Currently the code uses FunctionSamples::getCallSiteIdentifier which will sometimes incorrectly guess that FSAFDO discriminators are probe based and will convert them incorrectly.
This change doesn't affect builds which don't use FSAFDO, it only fixes sample profile matching with FS discriminators.
The test for this is manually updated to use discriminator value 15, which is a perfectly valid base discriminator in the FS world, but satisfies
isPseudoProbeDiscriminator
, sogetBaseDiscriminatorFromDiscriminator
will incorrectly extract the probe index from it.Note: this change only affects how the base discriminators will be extracted when doing stale profile matching in the IR-level sample profile loader. It doesn't add stale profile matching to the MIR-level FS profile loader pass.