[llvm][ctx_profile] Add the `llvm.instrprof.callsite` intrinsic #89939

mtrofin · 2024-04-24T15:18:24Z

Add the callsite intrinsic.

Structurally, it is very similar to the counter intrinsics, hence the inheritance relationship. We can probably rename InstrProfCntrInstBase to InstrProfIndexedBase later - because the "counting" aspect is really left to derived types of InstrProfCntrInstBase, and it only concerns itself with the index aspect (which is what we care about for callsite, too)

(Tracking Issue: #89287, RFC referenced there)

(Tracking Issue: llvm#89287, RFC referenced there)

llvmbot · 2024-04-24T15:18:58Z

@llvm/pr-subscribers-llvm-ir

Author: Mircea Trofin (mtrofin)

Changes

Add the callsite intrinsic.

Structurally, it is very similar to the counter intrinsics, hence the inheritance relationship. We can probably rename later InstrProfCntrInstBase to InstrProfIndexedBase later - because the "counting" aspect is really left to derived types of InstrProfCntrInstBase, and it only concerns itself with the index aspect (which is what we care about for callsite, too)

(Tracking Issue: #89287, RFC referenced there)

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

3 Files Affected:

(modified) llvm/include/llvm/IR/IntrinsicInst.h (+16)
(modified) llvm/include/llvm/IR/Intrinsics.td (+5)
(modified) llvm/unittests/IR/IntrinsicsTest.cpp (+2)

diff --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
index 4f22720f1c558d..8a137ffe28b9f8 100644
--- a/llvm/include/llvm/IR/IntrinsicInst.h
+++ b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -1435,6 +1435,7 @@ class InstrProfInstBase : public IntrinsicInst {
     case Intrinsic::instrprof_cover:
     case Intrinsic::instrprof_increment:
     case Intrinsic::instrprof_increment_step:
+    case Intrinsic::instrprof_callsite:
     case Intrinsic::instrprof_timestamp:
     case Intrinsic::instrprof_value_profile:
       return true;
@@ -1519,6 +1520,21 @@ class InstrProfIncrementInstStep : public InstrProfIncrementInst {
   }
 };
 
+/// This represents the llvm.instrprof.increment intrinsic.
+/// It is structurally like the increment or step counters, hence the
+/// inheritance relationship, albeit somewhat tenuous (it's not 'counting' per
+/// se)
+class InstrProfCallsite : public InstrProfCntrInstBase {
+public:
+  static bool classof(const IntrinsicInst *I) {
+    return I->getIntrinsicID() == Intrinsic::instrprof_callsite;
+  }
+  static bool classof(const Value *V) {
+    return isa<IntrinsicInst>(V) && classof(cast<IntrinsicInst>(V));
+  }
+  Value *getCallee() const;
+};
+
 /// This represents the llvm.instrprof.timestamp intrinsic.
 class InstrProfTimestampInst : public InstrProfCntrInstBase {
 public:
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 1d20f7e1b19854..a14e9dedef8c9e 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -914,6 +914,11 @@ def int_instrprof_increment_step : Intrinsic<[],
                                         [llvm_ptr_ty, llvm_i64_ty,
                                          llvm_i32_ty, llvm_i32_ty, llvm_i64_ty]>;
 
+// Callsite instrumentation for contextual profiling
+def int_instrprof_callsite : Intrinsic<[],
+                                        [llvm_ptr_ty, llvm_i64_ty,
+                                         llvm_i32_ty, llvm_i32_ty, llvm_ptr_ty]>;
+
 // A timestamp for instrumentation based profiling.
 def int_instrprof_timestamp : Intrinsic<[], [llvm_ptr_ty, llvm_i64_ty,
                                              llvm_i32_ty, llvm_i32_ty]>;
diff --git a/llvm/unittests/IR/IntrinsicsTest.cpp b/llvm/unittests/IR/IntrinsicsTest.cpp
index 3fa4b2cf73b6be..dddd2f73d4446b 100644
--- a/llvm/unittests/IR/IntrinsicsTest.cpp
+++ b/llvm/unittests/IR/IntrinsicsTest.cpp
@@ -81,6 +81,7 @@ TEST_F(IntrinsicsTest, InstrProfInheritance) {
   __ISA(InstrProfCoverInst, InstrProfCntrInstBase);
   __ISA(InstrProfIncrementInst, InstrProfCntrInstBase);
   __ISA(InstrProfIncrementInstStep, InstrProfIncrementInst);
+  __ISA(InstrProfCallsite, InstrProfCntrInstBase);
   __ISA(InstrProfTimestampInst, InstrProfCntrInstBase);
   __ISA(InstrProfValueProfileInst, InstrProfCntrInstBase);
   __ISA(InstrProfMCDCBitmapInstBase, InstrProfInstBase);
@@ -94,6 +95,7 @@ TEST_F(IntrinsicsTest, InstrProfInheritance) {
           {Intrinsic::instrprof_cover, isInstrProfCoverInst},
           {Intrinsic::instrprof_increment, isInstrProfIncrementInst},
           {Intrinsic::instrprof_increment_step, isInstrProfIncrementInstStep},
+          {Intrinsic::instrprof_callsite, isInstrProfCallsite},
           {Intrinsic::instrprof_mcdc_condbitmap_update,
            isInstrProfMCDCCondBitmapUpdate},
           {Intrinsic::instrprof_mcdc_parameters,

ellishg

Can you also update the docs?

llvm-project/llvm/docs/LangRef.rst

Lines 14142 to 14143 in 7c0da79

    
           '``llvm.instrprof.timestamp``' Intrinsic 
        
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

ellishg · 2024-04-24T15:45:16Z

llvm/include/llvm/IR/IntrinsicInst.h

@@ -1519,6 +1520,21 @@ class InstrProfIncrementInstStep : public InstrProfIncrementInst {
  }
 };

+/// This represents the llvm.instrprof.increment intrinsic.


Suggested change

/// This represents the llvm.instrprof.increment intrinsic.

/// This represents the llvm.instrprof.callsite intrinsic.

done - thanks for the catch!

mtrofin · 2024-04-24T17:57:07Z

Can you also update the docs?

llvm-project/llvm/docs/LangRef.rst

Lines 14142 to 14143 in 7c0da79

'``llvm.instrprof.timestamp``' Intrinsic

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

done. I'll also update the docs again once the ctx lowering pass is in, because the other intrinsics' docs are too specific. The doc for this one can become more precise at that point, too.

github-actions · 2024-04-24T21:25:30Z

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

snehasish · 2024-04-25T16:06:35Z

llvm/docs/LangRef.rst

+
+Arguments:
+""""""""""
+The first 3 arguments are similar to ``llvm.instrprof.increment``. The indexing


It would be good to have this documentation stand alone and describe the parameters here instead of relying on the reader to go look up the documentation of the llvm.instrprof.increment intrinsic.

It's the style of the rest of the documentation, see the intrinsics above.

We should take the opportunity to improve instead of following what exists. I think it's low overhead to include it in here inline.

Yes - I meant it as the typical pattern for this type of thing, didn't want to type that: either we fix the rest, too (separate change); or we think this is actually a good think for some reason (and then that separate change can be rejected). Either way, seems like a separate change.

snehasish · 2024-04-25T16:11:29Z

llvm/docs/LangRef.rst

+Overview:
+"""""""""
+
+The '``llvm.instrprof.callsite``' intrinsic should be emitted before a callsite


"should be emitted" -> can you specify under what modes this intrinsic is emitted (or is it always as this statement implies)?

I will update this after I introduce the pass.

Add a FIXME? Same for the other one.

snehasish · 2024-04-25T16:12:21Z

llvm/docs/LangRef.rst

+
+Semantics:
+""""""""""
+This is lowered by contextual profiling.


Can you expand this a bit to spell out how it is used (apart from just mentioning contextual profiling) like how it should be lowered, does it modify program behaviour, should it be treated as immovable by optimizations, when it should be ignored by backends and so on?

Same, I will need to update after I land the lowering pass.

snehasish · 2024-04-25T16:16:18Z

llvm/lib/IR/IntrinsicInst.cpp

@@ -291,6 +291,8 @@ Value *InstrProfIncrementInst::getStep() const {
  return ConstantInt::get(Type::getInt64Ty(Context), 1);
 }

+Value *InstrProfCallsite::getCallee() const { return getArgOperand(4); }


Do you need a check like L286 in getStep above to get the callee? If not can you add a comment.

good point, because nothing stops someone from reinterpret_cast-ing. Fixed.

snehasish

lgtm, with some minor nits.

snehasish · 2024-04-25T16:40:30Z

llvm/docs/LangRef.rst

+Overview:
+"""""""""
+
+The '``llvm.instrprof.callsite``' intrinsic should be emitted before a callsite


Add a FIXME? Same for the other one.

snehasish · 2024-04-25T16:42:12Z

llvm/docs/LangRef.rst

+
+Arguments:
+""""""""""
+The first 3 arguments are similar to ``llvm.instrprof.increment``. The indexing


We should take the opportunity to improve instead of following what exists. I think it's low overhead to include it in here inline.

[llvm][ctx_profile] Add the llvm.instrprof.callsite intrinsic

848deea

(Tracking Issue: llvm#89287, RFC referenced there)

mtrofin requested review from snehasish, ellishg, david-xl and xur-llvm April 24, 2024 15:18

llvmbot added the llvm:ir label Apr 24, 2024

mtrofin requested a review from teresajohnson April 24, 2024 15:19

ellishg reviewed Apr 24, 2024

View reviewed changes

fix comment, added LangRef.

622bba5

mtrofin force-pushed the callsite branch from 1d182b5 to 622bba5 Compare April 24, 2024 21:22

format

fca27fe

snehasish reviewed Apr 25, 2024

View reviewed changes

addressed some comments

b7a042d

snehasish approved these changes Apr 25, 2024

View reviewed changes

mtrofin added 2 commits April 25, 2024 10:00

fixme in docs

2a7dd97

Merge branch 'main' into callsite

a271f65

mtrofin merged commit ddb67e6 into llvm:main Apr 25, 2024
4 of 5 checks passed

mtrofin deleted the callsite branch April 25, 2024 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm][ctx_profile] Add the `llvm.instrprof.callsite` intrinsic #89939

[llvm][ctx_profile] Add the `llvm.instrprof.callsite` intrinsic #89939

mtrofin commented Apr 24, 2024 •

edited

Loading

llvmbot commented Apr 24, 2024

ellishg left a comment

ellishg Apr 24, 2024

mtrofin Apr 24, 2024

mtrofin commented Apr 24, 2024

github-actions bot commented Apr 24, 2024 •

edited

Loading

snehasish Apr 25, 2024

mtrofin Apr 25, 2024

snehasish Apr 25, 2024

mtrofin Apr 25, 2024

snehasish Apr 25, 2024

mtrofin Apr 25, 2024

snehasish Apr 25, 2024

mtrofin Apr 25, 2024

snehasish Apr 25, 2024

mtrofin Apr 25, 2024

mtrofin Apr 25, 2024

snehasish Apr 25, 2024

mtrofin Apr 25, 2024

snehasish left a comment

snehasish Apr 25, 2024

snehasish Apr 25, 2024

	'``llvm.instrprof.timestamp``' Intrinsic
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

	/// This represents the llvm.instrprof.increment intrinsic.
	/// This represents the llvm.instrprof.callsite intrinsic.

[llvm][ctx_profile] Add the llvm.instrprof.callsite intrinsic #89939

[llvm][ctx_profile] Add the llvm.instrprof.callsite intrinsic #89939

Conversation

mtrofin commented Apr 24, 2024 • edited Loading

llvmbot commented Apr 24, 2024

ellishg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrofin commented Apr 24, 2024

github-actions bot commented Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snehasish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[llvm][ctx_profile] Add the `llvm.instrprof.callsite` intrinsic #89939

[llvm][ctx_profile] Add the `llvm.instrprof.callsite` intrinsic #89939

mtrofin commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 24, 2024 •

edited

Loading