Skip to content

[IR][PGO] Verify the structure of VP metadata. #145584

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

Merged

Conversation

mtrofin
Copy link
Member

@mtrofin mtrofin commented Jun 24, 2025

No description provided.

Copy link
Member Author

mtrofin commented Jun 24, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

github-actions bot commented Jun 24, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_invalid_md_prof_metadata_on_instructions branch from 1464876 to 3cda0f3 Compare June 24, 2025 20:54
@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch from f144d7f to a9742de Compare June 24, 2025 20:54
@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_invalid_md_prof_metadata_on_instructions branch from 3cda0f3 to 8a91105 Compare June 24, 2025 23:05
@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch from a9742de to 02b36b7 Compare June 24, 2025 23:05
@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_invalid_md_prof_metadata_on_instructions branch from 8a91105 to 3c77cbc Compare June 25, 2025 15:08
@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch from 02b36b7 to 591bfc2 Compare June 25, 2025 15:08
Base automatically changed from users/mtrofin/06-24-_ir_pgo_verify_invalid_md_prof_metadata_on_instructions to main June 25, 2025 20:10
@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch from 591bfc2 to ff4c690 Compare June 25, 2025 20:13
Check(isValueProfileMD(MD),"invalid value profiling metadata",MD);
Check(isa<CallBase>(I),
"value profiling !prof metadata is expected to be placed on call "
"instructions (which may be memops)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't follow the memops comment. Can you elaborate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was WIP. Fixed.

@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch from ff4c690 to 32e82d0 Compare June 27, 2025 21:54
@mtrofin mtrofin marked this pull request as ready for review June 27, 2025 21:58
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-llvm-ir

Author: Mircea Trofin (mtrofin)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/145584.diff

3 Files Affected:

  • (modified) llvm/include/llvm/IR/ProfDataUtils.h (+3)
  • (modified) llvm/lib/IR/ProfDataUtils.cpp (+1-1)
  • (modified) llvm/lib/IR/Verifier.cpp (+16-2)
diff --git a/llvm/include/llvm/IR/ProfDataUtils.h b/llvm/include/llvm/IR/ProfDataUtils.h
index 5c0e08b03c245..c24b2aa19a407 100644
--- a/llvm/include/llvm/IR/ProfDataUtils.h
+++ b/llvm/include/llvm/IR/ProfDataUtils.h
@@ -35,6 +35,9 @@ LLVM_ABI bool hasProfMD(const Instruction &I);
 /// Checks if an MDNode contains Branch Weight Metadata
 LLVM_ABI bool isBranchWeightMD(const MDNode *ProfileData);
 
+/// Checks if an MDNode contains value profiling Metadata
+LLVM_ABI bool isValueProfileMD(const MDNode *ProfileData);
+
 /// Checks if an instructions has Branch Weight Metadata
 ///
 /// \param I The instruction to check
diff --git a/llvm/lib/IR/ProfDataUtils.cpp b/llvm/lib/IR/ProfDataUtils.cpp
index 740023ca6d23b..605208edda70a 100644
--- a/llvm/lib/IR/ProfDataUtils.cpp
+++ b/llvm/lib/IR/ProfDataUtils.cpp
@@ -103,7 +103,7 @@ bool isBranchWeightMD(const MDNode *ProfileData) {
   return isTargetMD(ProfileData, MDProfLabels::BranchWeights, MinBWOps);
 }
 
-static bool isValueProfileMD(const MDNode *ProfileData) {
+bool isValueProfileMD(const MDNode *ProfileData) {
   return isTargetMD(ProfileData, MDProfLabels::ValueProfile, MinVPOps);
 }
 
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 9cab88b09779a..26d3b8b49598b 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -112,6 +112,7 @@
 #include "llvm/IR/Value.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
+#include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/AMDGPUAddrSpace.h"
 #include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/Casting.h"
@@ -5026,9 +5027,22 @@ void Verifier::visitProfMetadata(Instruction &I, MDNode *MD) {
       Check(mdconst::dyn_extract<ConstantInt>(MDO),
             "!prof brunch_weights operand is not a const int");
     }
+  } else if (ProfName == MDProfLabels::ValueProfile) {
+    Check(isValueProfileMD(MD),"invalid value profiling metadata",MD);
+    ConstantInt *KindInt = mdconst::dyn_extract<ConstantInt>(MD->getOperand(1));
+    Check(KindInt, "VP !prof missing kind argument", MD);
+
+    auto Kind = KindInt->getZExtValue();
+    Check(Kind >= InstrProfValueKind::IPVK_First &&
+              Kind <= InstrProfValueKind::IPVK_Last,
+          "Invalid VP !prof kind", MD);
+    if (Kind == InstrProfValueKind::IPVK_IndirectCallTarget || Kind == InstrProfValueKind::IPVK_MemOPSize)
+      Check(isa<CallBase>(I),
+            "VP !prof indirect call or memop size expected to be applied to "
+            "CallBase instructions only",
+            MD);
   } else {
-    Check(ProfName == MDProfLabels::ValueProfile,
-          "expected either branch_weights or VP profile name", MD);
+    CheckFailed("expected either branch_weights or VP profile name", MD);
   }
 }
 

@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch 2 times, most recently from 7c6ce53 to 840f1d9 Compare June 30, 2025 14:51
@mtrofin mtrofin marked this pull request as draft June 30, 2025 14:53
@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch from 840f1d9 to 2f97403 Compare June 30, 2025 15:35
@mtrofin mtrofin marked this pull request as ready for review June 30, 2025 15:35
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for verifying the structure of “VP” (value profiling) metadata in LLVM IR, ensuring correct kinds, operand counts, and proper attachment points.

  • Introduce isValueProfileMD utility and expose it in the public header.
  • Implement checks in Verifier::visitProfMetadata for VP metadata kind, argument count, and placement.
  • Add new lit tests in value-profile.ll for valid and invalid VP metadata scenarios.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
llvm/test/Verifier/value-profile.ll Add tests for valid/invalid VP metadata kinds, counts, and use.
llvm/lib/IR/Verifier.cpp Include InstrProf.h, implement VP metadata validation logic, and update error handling.
llvm/lib/IR/ProfDataUtils.cpp Remove static to export isValueProfileMD for external use.
llvm/include/llvm/IR/ProfDataUtils.h Declare isValueProfileMD in the public API.
Comments suppressed due to low confidence (1)

llvm/lib/IR/Verifier.cpp:5048

  • The CheckFailed macro does not accept a metadata node argument; revert to using Check for consistency:
Check(ProfName == MDProfLabels::ValueProfile,
      "expected either branch_weights or VP profile name", MD);
    CheckFailed("expected either branch_weights or VP profile name", MD);

Copy link
Contributor

@teresajohnson teresajohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with a couple of small fixes noted below

@mtrofin mtrofin force-pushed the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch from 2f97403 to 63d1f53 Compare June 30, 2025 17:05
Copy link
Member Author

mtrofin commented Jun 30, 2025

Merge activity

  • Jun 30, 7:30 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 30, 7:31 PM UTC: @mtrofin merged this pull request with Graphite.

@mtrofin mtrofin merged commit 4662871 into main Jun 30, 2025
7 checks passed
@mtrofin mtrofin deleted the users/mtrofin/06-24-_ir_pgo_verify_the_structure_of_vp_metadata branch June 30, 2025 19:31
@@ -112,6 +112,7 @@
#include "llvm/IR/Value.h"
#include "llvm/InitializePasses.h"
#include "llvm/Pass.h"
#include "llvm/ProfileData/InstrProf.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially cyclic deps. Could we reorganize ProfileData or this?

rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants