[profcheck] Require unknown metadata have an origin parameter
#157594
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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
mtrofin
changed the title
unknown metadata have a origin parameterunknown metadata have an origin parameter
There was a problem hiding this comment.
What's the main purpose of doing this? This doesn't seem super relevant to the profcheck stuff, because that's catching places where the metadata was just completely ignored.
This also adds a bit of overhead (presumably only in an asserts build when profcheck is enabled) when this data could be obtained by bisecting the pass pipeline/in a debugger, although both of those are much less convenient.
This was referenced Sep 9, 2025
Does the description help? (I added approvers before writing and publishing it) |
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-llvm-transforms Author: Mircea Trofin (mtrofin) ChangesRather than passes using For example, suppose we emitted a We can also (in a subsequent pass) generate optimization remarks about such lowered selects, with a similar aim - identify patterns lowering to Full diff: https://github.com/llvm/llvm-project/pull/157594.diff 10 Files Affected:
diff --git a/llvm/include/llvm/IR/ProfDataUtils.h b/llvm/include/llvm/IR/ProfDataUtils.h
index b8386ddc86ca8..61434735506f9 100644
--- a/llvm/include/llvm/IR/ProfDataUtils.h
+++ b/llvm/include/llvm/IR/ProfDataUtils.h
@@ -177,12 +177,15 @@ inline uint32_t scaleBranchCount(uint64_t Count, uint64_t Scale) {
/// do not accidentally drop profile info, and this API is called in cases where
/// the pass explicitly cannot provide that info. Defaulting it in would hide
/// bugs where the pass forgets to transfer over or otherwise specify profile
-/// info.
-LLVM_ABI void setExplicitlyUnknownBranchWeights(Instruction &I);
+/// info. Use `PassName` to capture the pass name (i.e. DEBUG_TYPE) for
+/// debuggability.
+LLVM_ABI void setExplicitlyUnknownBranchWeights(Instruction &I,
+ StringRef PassName);
/// Analogous to setExplicitlyUnknownBranchWeights, but for functions and their
/// entry counts.
-LLVM_ABI void setExplicitlyUnknownFunctionEntryCount(Function &F);
+LLVM_ABI void setExplicitlyUnknownFunctionEntryCount(Function &F,
+ StringRef PassName);
LLVM_ABI bool isExplicitlyUnknownProfileMetadata(const MDNode &MD);
LLVM_ABI bool hasExplicitlyUnknownBranchWeights(const Instruction &I);
diff --git a/llvm/lib/IR/ProfDataUtils.cpp b/llvm/lib/IR/ProfDataUtils.cpp
index b41256f599096..d0b91d9356613 100644
--- a/llvm/lib/IR/ProfDataUtils.cpp
+++ b/llvm/lib/IR/ProfDataUtils.cpp
@@ -242,24 +242,26 @@ bool extractProfTotalWeight(const Instruction &I, uint64_t &TotalVal) {
return extractProfTotalWeight(I.getMetadata(LLVMContext::MD_prof), TotalVal);
}
-void setExplicitlyUnknownBranchWeights(Instruction &I) {
+void setExplicitlyUnknownBranchWeights(Instruction &I, StringRef PassName) {
MDBuilder MDB(I.getContext());
I.setMetadata(
LLVMContext::MD_prof,
MDNode::get(I.getContext(),
- MDB.createString(MDProfLabels::UnknownBranchWeightsMarker)));
+ {MDB.createString(MDProfLabels::UnknownBranchWeightsMarker),
+ MDB.createString(PassName)}));
}
-void setExplicitlyUnknownFunctionEntryCount(Function &F) {
+void setExplicitlyUnknownFunctionEntryCount(Function &F, StringRef PassName) {
MDBuilder MDB(F.getContext());
F.setMetadata(
LLVMContext::MD_prof,
MDNode::get(F.getContext(),
- MDB.createString(MDProfLabels::UnknownBranchWeightsMarker)));
+ {MDB.createString(MDProfLabels::UnknownBranchWeightsMarker),
+ MDB.createString(PassName)}));
}
bool isExplicitlyUnknownProfileMetadata(const MDNode &MD) {
- if (MD.getNumOperands() != 1)
+ if (MD.getNumOperands() != 2)
return false;
return MD.getOperand(0).equalsStr(MDProfLabels::UnknownBranchWeightsMarker);
}
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index f38871f09f35f..51c9955e3e6d1 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -631,7 +631,7 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
void verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
const Value *V, bool IsIntrinsic, bool IsInlineAsm);
void verifyFunctionMetadata(ArrayRef<std::pair<unsigned, MDNode *>> MDs);
-
+ void verifyUnknownProfileMetadata(MDNode *MD);
void visitConstantExprsRecursively(const Constant *EntryC);
void visitConstantExpr(const ConstantExpr *CE);
void visitConstantPtrAuth(const ConstantPtrAuth *CPA);
@@ -2515,19 +2515,31 @@ void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
V);
}
}
+void Verifier::verifyUnknownProfileMetadata(MDNode *MD) {
+ Check(MD->getNumOperands() == 2,
+ "'unknown' !prof should have a single additional operand", MD);
+ auto *PassName = dyn_cast<MDString>(MD->getOperand(1));
+ Check(PassName != nullptr,
+ "'unknown' !prof should have an additional operand of type "
+ "string");
+ Check(!PassName->getString().empty(),
+ "the 'unknown' !prof operand should not be an empty string");
+}
void Verifier::verifyFunctionMetadata(
ArrayRef<std::pair<unsigned, MDNode *>> MDs) {
for (const auto &Pair : MDs) {
if (Pair.first == LLVMContext::MD_prof) {
MDNode *MD = Pair.second;
+ Check(MD->getNumOperands() >= 2,
+ "!prof annotations should have no less than 2 operands", MD);
// We may have functions that are synthesized by the compiler, e.g. in
// WPD, that we can't currently determine the entry count.
- if (isExplicitlyUnknownProfileMetadata(*MD))
+ if (MD->getOperand(0).equalsStr(
+ MDProfLabels::UnknownBranchWeightsMarker)) {
+ verifyUnknownProfileMetadata(MD);
continue;
-
- Check(MD->getNumOperands() >= 2,
- "!prof annotations should have no less than 2 operands", MD);
+ }
// Check first operand.
Check(MD->getOperand(0) != nullptr, "first operand should not be null",
@@ -5054,8 +5066,7 @@ void Verifier::visitProfMetadata(Instruction &I, MDNode *MD) {
"'unknown' !prof should only appear on instructions on which "
"'branch_weights' would",
MD);
- Check(MD->getNumOperands() == 1,
- "'unknown' !prof should have no additional operands", MD);
+ verifyUnknownProfileMetadata(MD);
return;
}
diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
index bfb25c806e53f..09bffa7bf5846 100644
--- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
+++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
@@ -1495,7 +1495,7 @@ void DevirtModule::tryICallBranchFunnel(
if (!JT->getEntryCount().has_value()) {
// FIXME: we could pass through thinlto the necessary information.
- setExplicitlyUnknownFunctionEntryCount(*JT);
+ setExplicitlyUnknownFunctionEntryCount(*JT, DEBUG_TYPE);
}
}
diff --git a/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp b/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp
index b0478ee1fd94e..2025fbbf05973 100644
--- a/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp
@@ -186,7 +186,7 @@ expandToSwitch(CallBase *CB, const JumpTableTy &JT, DomTreeUpdater &DTU,
setBranchWeights(*Switch, downscaleWeights(BranchWeights),
/*IsExpected=*/false);
} else
- setExplicitlyUnknownBranchWeights(*Switch);
+ setExplicitlyUnknownBranchWeights(*Switch, DEBUG_TYPE);
if (PHI)
CB->replaceAllUsesWith(PHI);
CB->eraseFromParent();
diff --git a/llvm/test/Transforms/JumpTableToSwitch/basic.ll b/llvm/test/Transforms/JumpTableToSwitch/basic.ll
index e3c032b9d4bf7..c6f59406f7337 100644
--- a/llvm/test/Transforms/JumpTableToSwitch/basic.ll
+++ b/llvm/test/Transforms/JumpTableToSwitch/basic.ll
@@ -286,7 +286,7 @@ define i32 @function_with_jump_table_addrspace_42(i32 %index) addrspace(42) {
; CHECK: [[META0]] = !{i64 5678}
; CHECK: [[META1]] = !{i64 5555}
; CHECK: [[PROF2]] = !{!"branch_weights", i32 0, i32 20, i32 5}
-; CHECK: [[PROF3]] = !{!"unknown"}
+; CHECK: [[PROF3]] = !{!"unknown", !"jump-table-to-switch"}
;.
; THRESHOLD-0: [[META0]] = !{i64 5678}
; THRESHOLD-0: [[META1]] = !{i64 5555}
diff --git a/llvm/test/Transforms/PGOProfile/prof-inject-existing.ll b/llvm/test/Transforms/PGOProfile/prof-inject-existing.ll
index f51ec17d9166a..d2c12fb2670f9 100644
--- a/llvm/test/Transforms/PGOProfile/prof-inject-existing.ll
+++ b/llvm/test/Transforms/PGOProfile/prof-inject-existing.ll
@@ -14,9 +14,9 @@ no:
}
!0 = !{!"branch_weights", i32 1, i32 2}
-!1 = !{!"unknown"}
+!1 = !{!"unknown", !"test"}
; CHECK: define void @foo(i32 %i) !prof !0
; CHECK: br i1 %c, label %yes, label %no, !prof !1
; CHECK: !0 = !{!"function_entry_count", i64 1000}
; CHECK: !1 = !{!"branch_weights", i32 1, i32 2}
-; CHECK: !2 = !{!"unknown"}
+; CHECK: !2 = !{!"unknown", !"test"}
diff --git a/llvm/test/Transforms/PGOProfile/prof-verify-existing.ll b/llvm/test/Transforms/PGOProfile/prof-verify-existing.ll
index 793b221c4ea66..c3f0f6d3e44b4 100644
--- a/llvm/test/Transforms/PGOProfile/prof-verify-existing.ll
+++ b/llvm/test/Transforms/PGOProfile/prof-verify-existing.ll
@@ -15,9 +15,9 @@ no:
!0 = !{!"function_entry_count", i32 1}
!1 = !{!"branch_weights", i32 1, i32 2}
-!2 = !{!"unknown"}
+!2 = !{!"unknown", !"test"}
; CHECK: define void @foo(i32 %i) !prof !0
; CHECK: br i1 %c, label %yes, label %no, !prof !1
; CHECK: !0 = !{!"function_entry_count", i64 1}
; CHECK: !1 = !{!"branch_weights", i32 1, i32 2}
-; CHECK: !2 = !{!"unknown"}
+; CHECK: !2 = !{!"unknown", !"test"}
diff --git a/llvm/test/Transforms/WholeProgramDevirt/branch-funnel-profile.ll b/llvm/test/Transforms/WholeProgramDevirt/branch-funnel-profile.ll
index dd1aa926de8a5..f1a28ca8d681c 100644
--- a/llvm/test/Transforms/WholeProgramDevirt/branch-funnel-profile.ll
+++ b/llvm/test/Transforms/WholeProgramDevirt/branch-funnel-profile.ll
@@ -17,7 +17,7 @@
; NORETP: define hidden void @__typeid_typeid1_rv_0_branch_funnel(ptr nest %0, ...) !prof !11
; NORETP: define internal void @branch_funnel(ptr nest %0, ...) !prof !11
; NORETP: define internal void @branch_funnel.1(ptr nest %0, ...) !prof !11
-; NORETP: !11 = !{!"unknown"}
+; NORETP: !11 = !{!"unknown", !"wholeprogramdevirt"}
; O3: define hidden void @__typeid_typeid1_0_branch_funnel(ptr nest %0, ...) local_unnamed_addr #5 !prof !11
; O3: define hidden void @__typeid_typeid1_rv_0_branch_funnel(ptr nest %0, ...) local_unnamed_addr #5 !prof !11
@@ -27,7 +27,7 @@
; O3: define hidden void @__typeid_typeid3_rv_0_branch_funnel(ptr nest %0, ...) local_unnamed_addr #5 !prof !12
; O3: !10 = !{!"function_entry_count", i64 1000}
; O3: !11 = !{!"function_entry_count", i64 3000}
-; O3: !12 = !{!"unknown"}
+; O3: !12 = !{!"unknown", !"wholeprogramdevirt"}
target datalayout = "e-p:64:64"
target triple = "x86_64-unknown-linux-gnu"
diff --git a/llvm/test/Verifier/branch-weight.ll b/llvm/test/Verifier/branch-weight.ll
index d4f92c191e231..62d5357ed6d2c 100644
--- a/llvm/test/Verifier/branch-weight.ll
+++ b/llvm/test/Verifier/branch-weight.ll
@@ -11,6 +11,8 @@
; RUN: opt -passes=verify %t/unknown-correct.ll --disable-output
; RUN: not opt -passes=verify %t/unknown-invalid.ll --disable-output 2>&1 | FileCheck %s --check-prefix=EXTRA-ARGS
+; RUN: not opt -passes=verify %t/unknown-noargs.ll --disable-output 2>&1 | FileCheck %s --check-prefix=NO-ARGS
+; RUN: not opt -passes=verify %t/unknown-empty.ll --disable-output 2>&1 | FileCheck %s --check-prefix=EMPTY-ARGS
; RUN: opt -passes=verify %t/unknown-on-function1.ll -S -o - | FileCheck %s --check-prefix=ON-FUNCTION1
; RUN: not opt -passes=verify %t/unknown-on-function2.ll --disable-output 2>&1 | FileCheck %s --check-prefix=ON-FUNCTION2
; RUN: not opt -passes=verify %t/invalid-unknown-placement.ll --disable-output 2>&1 | FileCheck %s --check-prefix=INVALID-UNKNOWN-PLACEMENT
@@ -111,7 +113,7 @@ exit:
ret i32 %r
}
-!0 = !{!"unknown"}
+!0 = !{!"unknown", !"test"}
;--- unknown-invalid.ll
define void @test(i32 %a) {
@@ -124,14 +126,14 @@ no:
}
!0 = !{!"unknown", i32 12, i32 67}
-; EXTRA-ARGS: 'unknown' !prof should have no additional operands
+; EXTRA-ARGS: 'unknown' !prof should have a single additional operand
;--- unknown-on-function1.ll
define void @test() !prof !0 {
ret void
}
-!0 = !{!"unknown"}
+!0 = !{!"unknown", !"test"}
; ON-FUNCTION1: define void @test() !prof !0
;--- unknown-on-function2.ll
@@ -140,12 +142,38 @@ define void @test() !prof !0 {
}
!0 = !{!"unknown", i64 123}
-; ON-FUNCTION2: first operand should be 'function_entry_count' or 'synthetic_function_entry_count'
+; ON-FUNCTION2: 'unknown' !prof should have an additional operand of type string
;--- invalid-unknown-placement.ll
define i32 @test() {
%r = add i32 1, 2, !prof !0
ret i32 %r
}
-!0 = !{!"unknown"}
+!0 = !{!"unknown", !"test"}
; INVALID-UNKNOWN-PLACEMENT: 'unknown' !prof should only appear on instructions on which 'branch_weights' would
+
+;--- unknown-noargs.ll
+define void @test(i32 %a) {
+ %c = icmp eq i32 %a, 0
+ br i1 %c, label %yes, label %no, !prof !0
+yes:
+ ret void
+no:
+ ret void
+}
+
+!0 = !{!"unknown"}
+; NO-ARGS: 'unknown' !prof should have a single additional operand
+
+;--- unknown-empty.ll
+define void @test(i32 %a) {
+ %c = icmp eq i32 %a, 0
+ br i1 %c, label %yes, label %no, !prof !0
+yes:
+ ret void
+no:
+ ret void
+}
+
+!0 = !{!"unknown", !""}
+; EMPTY-ARGS: the 'unknown' !prof operand should not be an empty string
|
boomanaiden154
approved these changes
Sep 9, 2025
snehasish
reviewed
Sep 9, 2025
david-xl
reviewed
Sep 9, 2025
| } | ||
|
|
||
| !0 = !{!"unknown", i64 123} | ||
| ; ON-FUNCTION2: first operand should be 'function_entry_count' or 'synthetic_function_entry_count' |
There was a problem hiding this comment.
What happens to this error message (about entry count)?
There was a problem hiding this comment.
It is a different check -- the original test case intends to check entry count being the first one, so perhaps change this test to have value second operand.
mtrofin
deleted the
users/mtrofin/09-09-_profcheck_require_unknown_metadata_have_a_origin_parameter
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/30859 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Rather than passes using
!prof = !{!”unknown”}for cases where don’t have enough information to emit profile values, this patch captures the pass (or some other information) that can help diagnostics - i.e.!{!”unknown”, !”some-pass-name”}.For example, suppose we emitted a
select with the unknown metadata, and, later, end up needing to lower that to a conditional branch. If we observe (via sample profiling, for example) that the branch is biased and would have benefitted from a valid profile, the extra information can help speed up debugging.We can also (in a subsequent pass) generate optimization remarks about such lowered selects, with a similar aim - identify patterns lowering to
select that may be worth some extra investment in extracting a more precise profile.