Skip to content

[nfc] Fix RTTI for InstrProf intrinsics #83511

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
merged 1 commit into from
Mar 6, 2024
Merged

[nfc] Fix RTTI for InstrProf intrinsics #83511

merged 1 commit into from
Mar 6, 2024

Conversation

mtrofin
Copy link
Member

@mtrofin mtrofin commented Mar 1, 2024

No description provided.

@mtrofin mtrofin requested a review from ellishg March 1, 2024 01:08
@llvmbot llvmbot added the llvm:ir label Mar 1, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 1, 2024

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-ir

Author: Mircea Trofin (mtrofin)

Changes

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

1 Files Affected:

  • (modified) llvm/include/llvm/IR/IntrinsicInst.h (+16)
diff --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
index fbaaef8ea44315..2bd4f94f53170e 100644
--- a/llvm/include/llvm/IR/IntrinsicInst.h
+++ b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -1428,8 +1428,15 @@ class VACopyInst : public IntrinsicInst {
 };
 
 /// A base class for all instrprof intrinsics.
+class InstrProfInstBase;
+class InstrProfMCDCBitmapInstBase;
+class InstrProfMCDCCondBitmapUpdate;
 class InstrProfInstBase : public IntrinsicInst {
 public:
+  static bool classof(const Value *V) {
+    return isa<InstrProfInstBase>(V) || isa<InstrProfMCDCBitmapInstBase>(V) ||
+           isa<InstrProfMCDCCondBitmapUpdate>(V);
+  }
   // The name of the instrumented function.
   GlobalVariable *getName() const {
     return cast<GlobalVariable>(
@@ -1442,8 +1449,17 @@ class InstrProfInstBase : public IntrinsicInst {
 };
 
 /// A base class for all instrprof counter intrinsics.
+class InstProfCoverInst;
+class InstProfIncrementInst;
+class InstrProfTimestampInst;
+class InstrProfValueProfileInst;
 class InstrProfCntrInstBase : public InstrProfInstBase {
 public:
+  static bool classof(const Value *V){
+    return isa<InstProfCoverInst>(V) || isa<InstProfIncrementInst>(V) ||
+           isa<InstrProfTimestampInst>(V) || isa<InstrProfValueProfileInst>(V);
+  }
+
   // The number of counters for the instrumented function.
   ConstantInt *getNumCounters() const;
   // The index of the counter that this instruction acts on.

Copy link

github-actions bot commented Mar 1, 2024

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

@mtrofin mtrofin changed the title [mfc] Fix RTTI for InstrProf intrinsics [nfc] Fix RTTI for InstrProf intrinsics Mar 1, 2024
@llvmbot llvmbot added PGO Profile Guided Optimizations llvm:transforms labels Mar 1, 2024
…#83364)

Simpler code, compared to tracking state of 2 variables and the ambiguity of "0" CountValue (is it 0 or is it invalid?)
@mtrofin
Copy link
Member Author

mtrofin commented Mar 5, 2024

@ellishg - is this good to go? thanks!

if (const auto *Instr = dyn_cast<IntrinsicInst>(V))
return isCounterBase(*Instr) || isMCDCBitmapBase(*Instr) ||
Instr->getIntrinsicID() ==
Intrinsic::instrprof_mcdc_condbitmap_update;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be in isMCDCBitmapBase() CC @evodius96

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I found the comment for InstrProfMCDCCondBitmapUpdate

Copy link
Contributor

@ellishg ellishg left a comment

Choose a reason for hiding this comment

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

LGTM

@mtrofin mtrofin merged commit 3f7aa04 into llvm:main Mar 6, 2024
@mtrofin mtrofin deleted the rtti branch March 6, 2024 05:11
mtrofin added a commit to mtrofin/llvm-project that referenced this pull request Apr 20, 2024
Following up PR llvm#83511, added a test to cover the inheritance graph for these intrinsics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:ir llvm:transforms PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants