[PseudoProbe] Make probe discriminator compatible with dwarf base discriminator #94506
Conversation
This file contains hidden or 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
wlei-llvm
force-pushed
the
fix-discriminator
branch
from
ba8361c to
6217a7b
Compare
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-pgo Author: Lei Wang (wlei-llvm) ChangesIt's useful if the probe-based build can consume a dwarf based profile(e.g. the profile transition), before there is a conflict for the discriminator, this change tries to mitigate the issue by encoding the dwarf base discriminator into the probe discriminator.
Note that these doesn't affect the original probe id capacity, we still prioritize probe id encoding, i.e. the base discriminator is not encoded when probe id is bigger than [15:3]. Then adjust Full diff: https://github.com/llvm/llvm-project/pull/94506.diff 10 Files Affected:
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 18873a551595a..4515f5db8e63d 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2147,13 +2147,18 @@ class DILocation : public MDNode {
static unsigned
getBaseDiscriminatorFromDiscriminator(unsigned D,
bool IsFSDiscriminator = false) {
- // Return the probe id instead of zero for a pseudo probe discriminator.
- // This should help differenciate callsites with same line numbers to
- // achieve a decent AutoFDO profile under -fpseudo-probe-for-profiling,
- // where the original callsite dwarf discriminator is overwritten by
- // callsite probe information.
- if (isPseudoProbeDiscriminator(D))
- return PseudoProbeDwarfDiscriminator::extractProbeIndex(D);
+ // Extract the dwarf base discriminator if it's encoded in the pseudo probe
+ // discriminator. Otherwise, return the probe id instead of zero for a
+ // pseudo probe discriminator. This should help differenciate callsites with
+ // same line numbers to achieve a decent AutoFDO profile under
+ // -fpseudo-probe-for-profiling, where the original callsite dwarf
+ // discriminator is overwritten by callsite probe information.
+ if (isPseudoProbeDiscriminator(D)) {
+ if (PseudoProbeDwarfDiscriminator::isDwarfBaseDiscriminatorEncoded(D))
+ return PseudoProbeDwarfDiscriminator::extractDwarfBaseDiscriminator(D);
+ else
+ return PseudoProbeDwarfDiscriminator::extractProbeIndex(D);
+ }
if (IsFSDiscriminator)
return getMaskedDiscriminator(D, getBaseDiscriminatorBits());
diff --git a/llvm/include/llvm/IR/PseudoProbe.h b/llvm/include/llvm/IR/PseudoProbe.h
index cdbd498a8be61..23e597efae9a7 100644
--- a/llvm/include/llvm/IR/PseudoProbe.h
+++ b/llvm/include/llvm/IR/PseudoProbe.h
@@ -44,26 +44,52 @@ struct PseudoProbeDwarfDiscriminator {
// 32-bit integer which is organized as:
// [2:0] - 0x7, this is reserved for regular discriminator,
// see DWARF discriminator encoding rule
- // [18:3] - probe id
+ // if the [28:28] bit is zero:
+ // [18:3] for probe id.
+ // else:
+ // [15:3] for probe id, [18:16] for dwarf base discriminator.
// [25:19] - probe distribution factor
- // [28:26] - probe type, see PseudoProbeType
- // [31:29] - reserved for probe attributes
+ // [27:26] - probe type, see PseudoProbeType
+ // [28:28] - indicates whether dwarf base discriminator is encoded.
+ // [30:29] - reserved for probe attributes
static uint32_t packProbeData(uint32_t Index, uint32_t Type, uint32_t Flags,
- uint32_t Factor) {
+ uint32_t Factor,
+ uint32_t DwarfBaseDiscriminator) {
assert(Index <= 0xFFFF && "Probe index too big to encode, exceeding 2^16");
- assert(Type <= 0x7 && "Probe type too big to encode, exceeding 7");
+ assert(Type <= 0x3 && "Probe type too big to encode, exceeding 3");
assert(Flags <= 0x7);
assert(Factor <= 100 &&
"Probe distribution factor too big to encode, exceeding 100");
- return (Index << 3) | (Factor << 19) | (Type << 26) | 0x7;
+ uint32_t V = (Index << 3) | (Factor << 19) | (Type << 26) | 0x7;
+ // If both the probe id and dwarf base discriminator is small, the probe id
+ // space is shared with the dwarf base discriminator, this is to make the
+ // probe-based build to be compatible with the dwarf-based profile. Pack the
+ // dwarf base discriminator into [18:16] and set the [28:28] bit.
+ if (Index <= 0x1FFF && DwarfBaseDiscriminator <= 0x7)
+ V |= (1 << 28) | (DwarfBaseDiscriminator << 16);
+ return V;
}
static uint32_t extractProbeIndex(uint32_t Value) {
+ if (isDwarfBaseDiscriminatorEncoded(Value))
+ return (Value >> 3) & 0x1FFF;
return (Value >> 3) & 0xFFFF;
}
+ static uint32_t extractDwarfBaseDiscriminator(uint32_t Value) {
+ if (isDwarfBaseDiscriminatorEncoded(Value))
+ return (Value >> 16) & 0x7;
+ // Return an invalid value to indicate the dwarf base discriminator is not
+ // encoded.
+ return 0xFFFFFFFF;
+ }
+
+ static uint32_t isDwarfBaseDiscriminatorEncoded(uint32_t Value) {
+ return (Value >> 28) & 0x1;
+ }
+
static uint32_t extractProbeType(uint32_t Value) {
- return (Value >> 26) & 0x7;
+ return (Value >> 26) & 0x3;
}
static uint32_t extractProbeAttributes(uint32_t Value) {
diff --git a/llvm/lib/IR/PseudoProbe.cpp b/llvm/lib/IR/PseudoProbe.cpp
index df5f78c511822..7d3da0dd6b76c 100644
--- a/llvm/lib/IR/PseudoProbe.cpp
+++ b/llvm/lib/IR/PseudoProbe.cpp
@@ -95,13 +95,21 @@ void setProbeDistributionFactor(Instruction &Inst, float Factor) {
PseudoProbeDwarfDiscriminator::extractProbeType(Discriminator);
auto Attr = PseudoProbeDwarfDiscriminator::extractProbeAttributes(
Discriminator);
+ auto IsDwarfBaseDiscriminatorEncoded =
+ PseudoProbeDwarfDiscriminator::isDwarfBaseDiscriminatorEncoded(
+ Discriminator);
+ auto DwarfBaseDiscriminator =
+ PseudoProbeDwarfDiscriminator::extractDwarfBaseDiscriminator(
+ Discriminator);
// Round small factors to 0 to avoid over-counting.
uint32_t IntFactor =
PseudoProbeDwarfDiscriminator::FullDistributionFactor;
if (Factor < 1)
IntFactor *= Factor;
uint32_t V = PseudoProbeDwarfDiscriminator::packProbeData(
- Index, Type, Attr, IntFactor);
+ Index, Type, Attr, IntFactor,
+ IsDwarfBaseDiscriminatorEncoded ? DwarfBaseDiscriminator
+ : 0xFFFFFFFF);
DIL = DIL->cloneWithDiscriminator(V);
Inst.setDebugLoc(DIL);
}
diff --git a/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp b/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
index 9a191b0e38bea..3daa05a76d364 100644
--- a/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
@@ -431,8 +431,8 @@ void SampleProfileProber::instrumentOneFunc(Function &F, TargetMachine *TM) {
// and type of a callsite probe. This gets rid of the dependency on
// plumbing a customized metadata through the codegen pipeline.
uint32_t V = PseudoProbeDwarfDiscriminator::packProbeData(
- Index, Type, 0,
- PseudoProbeDwarfDiscriminator::FullDistributionFactor);
+ Index, Type, 0, PseudoProbeDwarfDiscriminator::FullDistributionFactor,
+ DIL->getBaseDiscriminator());
DIL = DIL->cloneWithDiscriminator(V);
Call->setDebugLoc(DIL);
}
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-discriminator.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-discriminator.ll
index 97b0ed600ad10..6d4429b37a52b 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-discriminator.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-discriminator.ll
@@ -64,8 +64,8 @@ attributes #1 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "fra
; PROBE: ![[CALL1]] = !DILocation(line: 4, column: 3, scope: ![[CALL1BLOCK:[0-9]+]])
-; PROBE: ![[CALL1BLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 186646559)
+; PROBE: ![[CALL1BLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 455147551)
; PROBE: ![[CALL2]] = !DILocation(line: 4, column: 9, scope: ![[CALL2BLOCK:[0-9]+]])
-; PROBE: ![[CALL2BLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 186646567)
+; PROBE: ![[CALL2BLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 455344167)
; PROBE: ![[INST]] = !DILocation(line: 4, column: 15, scope: ![[INSTBLOCK:[0-9]+]])
; PROBE: ![[INSTBLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 4)
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-emit-inline.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-emit-inline.ll
index 3d278a9ba27fd..0bde361018f7d 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-emit-inline.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-emit-inline.ll
@@ -42,13 +42,13 @@ define dso_local i32 @entry() !dbg !14 {
; CHECK-IL: ![[#SCOPE2:]] = distinct !DISubprogram(name: "foo"
; CHECK-IL: ![[#DL1]] = !DILocation(line: 3, column: 1, scope: ![[#SCOPE1]], inlinedAt: ![[#INL1:]])
; CHECK-IL: ![[#INL1]] = distinct !DILocation(line: 7, column: 3, scope: ![[#BL1:]])
-;; A discriminator of 186646551 which is 0xb200017 in hexdecimal, stands for a direct call probe
+;; A discriminator of 455082007 which is 0x1b200017 in hexdecimal, stands for a direct call probe
;; with an index of 2 and a scale of 100%.
-; CHECK-IL: ![[#BL1]] = !DILexicalBlockFile(scope: ![[#SCOPE2]], file: !1, discriminator: 186646551)
+; CHECK-IL: ![[#BL1]] = !DILexicalBlockFile(scope: ![[#SCOPE2]], file: !1, discriminator: 455082007)
; CHECK-IL: ![[#SCOPE3:]] = distinct !DISubprogram(name: "entry"
; CHECK-IL: ![[#DL2]] = !DILocation(line: 7, column: 3, scope: ![[#SCOPE2]], inlinedAt: ![[#INL2:]])
; CHECK-IL: ![[#INL2]] = distinct !DILocation(line: 11, column: 3, scope: ![[#BL2:]])
-; CHECK-IL: ![[#BL2]] = !DILexicalBlockFile(scope: ![[#SCOPE3]], file: !1, discriminator: 186646551)
+; CHECK-IL: ![[#BL2]] = !DILexicalBlockFile(scope: ![[#SCOPE3]], file: !1, discriminator: 455082007)
; CHECK-IL: ![[#DL3]] = !DILocation(line: 3, column: 1, scope: ![[#SCOPE1]], inlinedAt: ![[#INL3:]])
; CHECK-IL: ![[#INL3]] = distinct !DILocation(line: 7, column: 3, scope: ![[#BL1]], inlinedAt: ![[#INL2]])
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-emit.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-emit.ll
index 13cfd820ae82c..62d3d8255a175 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-emit.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-emit.ll
@@ -84,13 +84,13 @@ entry:
; CHECK-IL: ![[#REALLINE]] = !DILocation(line: 2, scope: ![[#DISC0:]])
; CHECK-IL: ![[#DISC0]] = !DILexicalBlockFile(scope: ![[#FOO]], file: ![[#]], discriminator: 0)
; CHECK-IL: ![[#PROBE0]] = !DILocation(line: 2, column: 20, scope: ![[#SCOPE0:]])
-;; A discriminator of 67108887 which is 0x7200017 in hexdecimal, stands for a direct call probe
+;; A discriminator of 387973143 which is 0x17200017 in hexdecimal, stands for a direct call probe
;; with an index of 2.
-; CHECK-IL: ![[#SCOPE0]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 119537687)
+; CHECK-IL: ![[#SCOPE0]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 387973143)
; CHECK-IL: ![[#PROBE1]] = !DILocation(line: 0, scope: ![[#SCOPE1:]])
-;; A discriminator of 186646559 which is 0xb20001f in hexdecimal, stands for a direct call probe
+;; A discriminator of 455082015 which is 0x1b20001f in hexdecimal, stands for a direct call probe
;; with an index of 3.
-; CHECK-IL: ![[#SCOPE1]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 186646559)
+; CHECK-IL: ![[#SCOPE1]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 455082015)
; Check the generation of .pseudo_probe_desc section
; CHECK-ASM: .section .pseudo_probe_desc,"G",@progbits,.pseudo_probe_desc_foo,comdat
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll
index 2cd9abf0e11e9..df3bc61d02520 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll
@@ -106,8 +106,6 @@ if.end:
;YAML-NEXT: - Line: '1'
;YAML-NEXT: - String: ':'
;YAML-NEXT: - Column: '11'
-;YAML-NEXT: - String: .
-;YAML-NEXT: - Disc: '2'
;YAML-NEXT: - String: ';'
;YAML-NEXT: ...
;YAML: --- !Analysis
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile.ll
index 7258ffca1278f..b52f93763d492 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-profile.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-profile.ll
@@ -40,14 +40,14 @@ attributes #0 = {"use-sample-profile"}
; CHECK: ![[PD1]] = !{!"branch_weights", i32 8, i32 7}
; CHECK: ![[#PROBE1]] = !DILocation(line: 0, scope: ![[#SCOPE1:]])
-;; A discriminator of 119537695 which is 0x720001f in hexdecimal, stands for an indirect call probe
+;; A discriminator of 387973151 which is 0x1720001f in hexdecimal, stands for an indirect call probe
;; with an index of 3.
-; CHECK: ![[#SCOPE1]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 119537695)
+; CHECK: ![[#SCOPE1]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 387973151)
; CHECK: ![[PROF1]] = !{!"VP", i32 0, i64 7, i64 9191153033785521275, i64 5, i64 -1069303473483922844, i64 2}
-;; A discriminator of 119537711 which is 0x720002f in hexdecimal, stands for an indirect call probe
+;; A discriminator of 387973167 which is 0x1720002f in hexdecimal, stands for an indirect call probe
;; with an index of 5.
; CHECK: ![[#PROBE2]] = !DILocation(line: 0, scope: ![[#SCOPE2:]])
-; CHECK: ![[#SCOPE2]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 119537711)
+; CHECK: ![[#SCOPE2]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 387973167)
; CHECK: ![[PROF2]] = !{!"VP", i32 0, i64 6, i64 -1069303473483922844, i64 4, i64 9191153033785521275, i64 2}
!llvm.module.flags = !{!9, !10}
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-verify.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-verify.ll
index b622cfbd6634e..dccd37e9de99d 100644
--- a/llvm/test/Transforms/SampleProfile/pseudo-probe-verify.ll
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-verify.ll
@@ -54,10 +54,10 @@ bb24:
ret void
}
-;; A discriminator of 186646583 which is 0xb200037 in hexdecimal, stands for a direct call probe
+;; A discriminator of 455082031 which is 0x1b200037 in hexdecimal, stands for a direct call probe
;; with an index of 6 and a scale of -1%.
; CHECK: ![[#PROBE6]] = !DILocation(line: 2, column: 20, scope: ![[#SCOPE:]])
-; CHECK: ![[#SCOPE]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 186646575)
+; CHECK: ![[#SCOPE]] = !DILexicalBlockFile(scope: ![[#]], file: ![[#]], discriminator: 455082031)
!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!9, !10}
|
WenleiHe
reviewed
Jun 5, 2024
llvm/include/llvm/IR/PseudoProbe.h
Outdated
| return 0xFFFFFFFF; | ||
| } | ||
|
|
||
| static uint32_t isDwarfBaseDiscriminatorEncoded(uint32_t Value) { |
There was a problem hiding this comment.
Given this is is..., should this return a boolean, and just check Value & mask?
There was a problem hiding this comment.
Good catch, fixed.
llvm/include/llvm/IR/PseudoProbe.h
Outdated
| return (Value >> 16) & 0x7; | ||
| // Return an invalid value to indicate the dwarf base discriminator is not | ||
| // encoded. | ||
| return 0xFFFFFFFF; |
There was a problem hiding this comment.
Should we always expect isDwarfBaseDiscriminatorEncoded be checked before calling extractDwarfBaseDiscriminator? Basically turning this into an assertion.
There was a problem hiding this comment.
I just changed the return type to optional, it's required to check in the caller, then we don't need the special 0xFFFFFFFF.
WenleiHe
reviewed
Jun 5, 2024
llvm/include/llvm/IR/PseudoProbe.h
Outdated
| uint32_t V = (Index << 3) | (Factor << 19) | (Type << 26) | 0x7; | ||
| // If both the probe id and dwarf base discriminator is small, the probe id | ||
| // space is shared with the dwarf base discriminator, this is to make the | ||
| // probe-based build to be compatible with the dwarf-based profile. Pack the |
Tested on our internal services, both pass1 and pass2 are good.
Dumped the stats of this. So the 3 bits ( <= 7) discriminator account for roughly 96% of the total discriminator. |
nekoshirro
pushed a commit
to nekoshirro/Alchemist-LLVM
that referenced
this pull request
Jun 9, 2024
…criminator (llvm#94506) It's useful if the probe-based build can consume a dwarf based profile(e.g. the profile transition), before there is a conflict for the discriminator, this change tries to mitigate the issue by encoding the dwarf base discriminator into the probe discriminator. As the num of probe id(num of basic block and calls) starts from 1, there are some unused space. We try to reuse some bit of the probe id. The new encode rule is: - Use a bit to [28:28] to indicate whether dwarf base discriminator is encoded.(fortunately we can borrow this bit from the `PseudoProbeType`) - If the bit is set, use [15:3] for probe id, [18:16] for dwarf base discriminator. Otherwise, still use [18:3] for probe id. Note that these doesn't affect the original probe id capacity, we still prioritize probe id encoding, i.e. the base discriminator is not encoded when probe id is bigger than [15:3]. Then adjust `getBaseDiscriminatorFromDiscriminator` to use the base discriminator from the probe discriminator. Signed-off-by: Hafidz Muzakky <[email protected]>
Closed
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.
It's useful if the probe-based build can consume a dwarf based profile(e.g. the profile transition), before there is a conflict for the discriminator, this change tries to mitigate the issue by encoding the dwarf base discriminator into the probe discriminator.
As the num of probe id(num of basic block and calls) starts from 1, there are some unused space. We try to reuse some bit of the probe id.
The new encode rule is:
PseudoProbeType)Note that these doesn't affect the original probe id capacity, we still prioritize probe id encoding, i.e. the base discriminator is not encoded when probe id is bigger than [15:3].
Then adjust
getBaseDiscriminatorFromDiscriminatorto use the base discriminator from the probe discriminator.