Skip to content
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

[IR] Introduce llvm.allow.{runtime,ubsan}.check() #84850

Merged

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Mar 11, 2024

The goal is to have ability to change logic compile time based on PGO data.

Our primary application is removing UBSAN checks from hot code.
Then we'd like to use this for libc++ hardening and regular debug asserts.
Previous attempt is #84214.

Benefits from special intrinsic vs #84214:

  1. Resulting binary is 3% faster than removing traps (on "test-suite/MultiSource/Benchmarks" with PGO+ThinLTO)
  2. Intrinsic can be used from source code to change behavior from C/C++ program. E.g. enabling asserts in cold code.
  3. Easier to match basic blocks.

RFC: https://discourse.llvm.org/t/rfc-add-llvm-experimental-hot-intrinsic-or-llvm-hot/77641

@llvmbot llvmbot added llvm:SelectionDAG SelectionDAGISel as well llvm:ir labels Mar 11, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 11, 2024

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-selectiondag

Author: Vitaly Buka (vitalybuka)

Changes

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

4 Files Affected:

  • (modified) llvm/docs/LangRef.rst (+48)
  • (modified) llvm/include/llvm/IR/Intrinsics.td (+5)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+6)
  • (added) llvm/test/CodeGen/Generic/builtin-hot.ll (+19)
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index d613ceea8654f8..36f4c964ee296c 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -27639,6 +27639,54 @@ constant `true`. However it is always correct to replace
 it with any other `i1` value. Any pass can
 freely do it if it can benefit from non-default lowering.
 
+'``llvm.experimental.hot``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Syntax:
+"""""""
+
+::
+
+      declare i1 @llvm.experimental.hot()
+
+Overview:
+"""""""""
+
+This intrinsic returns true iff it's known that containing basic block is hot in
+profile.
+
+When used with profile based optimization allows to change program behaviour
+deppending on the code hotness.
+
+Arguments:
+""""""""""
+
+None.
+
+Semantics:
+""""""""""
+
+The intrinsic ``@llvm.experimental.hot()`` returns either `true` or `false`,
+deppending on profile used. Expresion is evaluated as `true` iff profile and
+summary are availible and profile counter for the block reach hotness threshold.
+For each evaluation of a call to this intrinsic, the program must be valid and
+correct both if it returns `true` and if it returns `false`.
+
+When used in a branch condition, it allows us to choose between
+two alternative correct solutions for the same problem, like
+in example below:
+
+.. code-block:: text
+
+    %cond = call i1 @llvm.experimental.hot()
+    br i1 %cond, label %fast_path, label %slow_path
+
+  label %fast_path:
+    ; Omit diagnostics.
+
+  label %slow_path:
+    ; Additional diagnostics.
+
 
 '``llvm.load.relative``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 144298fd7c0162..96e9cdf6627a75 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1722,6 +1722,11 @@ def int_debugtrap : Intrinsic<[]>,
 def int_ubsantrap : Intrinsic<[], [llvm_i8_ty],
                               [IntrNoReturn, IntrCold, ImmArg<ArgIndex<0>>]>;
 
+// Return true if profile counter for containing block is hot.
+def int_experimental_hot : Intrinsic<[llvm_i1_ty], [],
+                                      [IntrInaccessibleMemOnly, IntrWriteMem,
+                                       IntrWillReturn, NoUndef<RetIndex>]>;
+
 // Support for dynamic deoptimization (or de-specialization)
 def int_experimental_deoptimize : Intrinsic<[llvm_any_ty], [llvm_vararg_ty],
                                             [Throws]>;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 22e57d0d99e9b1..8e73433ce82ea5 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7276,6 +7276,12 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
     setValue(&I, getValue(I.getArgOperand(0)));
     return;
 
+  case Intrinsic::experimental_hot:
+    // Default lowering to false. It's intended to be lowered as soon as profile
+    // is avalible to unblock other optimizations.
+    setValue(&I, DAG.getConstant(0, sdl, MVT::i1));
+    return;
+
   case Intrinsic::ubsantrap:
   case Intrinsic::debugtrap:
   case Intrinsic::trap: {
diff --git a/llvm/test/CodeGen/Generic/builtin-hot.ll b/llvm/test/CodeGen/Generic/builtin-hot.ll
new file mode 100644
index 00000000000000..449f58d3c00675
--- /dev/null
+++ b/llvm/test/CodeGen/Generic/builtin-hot.ll
@@ -0,0 +1,19 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -o - %s | FileCheck %s
+
+; REQUIRES: aarch64-registered-target
+
+target triple = "aarch64-linux"
+
+define i1 @test()  {
+; CHECK-LABEL: test:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    mov w0, wzr
+; CHECK-NEXT:    ret
+entry:
+  %hot = call i1 @llvm.experimental.hot()
+  ret i1 %hot
+}
+
+declare i1 @llvm.expect.hot() nounwind
+

@jroelofs
Copy link
Contributor

Neat idea!

@aeubanks
Copy link
Contributor

+1, this is a cool addition!

llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
@nikic
Copy link
Contributor

nikic commented Mar 12, 2024

Please submit an RFC on discourse for this change.

Co-authored-by: Nikita Popov <npopov@redhat.com>
@vitalybuka
Copy link
Collaborator Author

Please submit an RFC on discourse for this change.

https://discourse.llvm.org/t/rfc-add-llvm-experimental-hot-intrinsic-or-llvm-hot/77641

Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.ir-introduce-llvmexperimentalhot to main March 13, 2024 00:56
Created using spr 1.3.4
@preames preames removed their request for review March 13, 2024 03:54
@vitalybuka vitalybuka changed the title [IR] Introduce llvm.experimental.hot() [IR] Introduce llvm.allow.{runtime,ubsan}.check() Mar 19, 2024
Created using spr 1.3.4
Created using spr 1.3.4
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
Created using spr 1.3.4
Created using spr 1.3.4
@vitalybuka vitalybuka requested review from arsenm and nikic March 20, 2024 21:21
Created using spr 1.3.4
Created using spr 1.3.4
@vitalybuka
Copy link
Collaborator Author

How can we move forward here?

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

I might have used allow -> should in the name, but this looks ok as well.

llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
Created using spr 1.3.4
@vitalybuka
Copy link
Collaborator Author

@efriedma-quic @arsenm Would you like me to wait for your approval?

llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
@arsenm
Copy link
Contributor

arsenm commented Mar 27, 2024

@efriedma-quic @arsenm Would you like me to wait for your approval?

No

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

Didn't review carefully, but looks fine.

Created using spr 1.3.4
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit 90c738e into main Apr 1, 2024
4 of 5 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/ir-introduce-llvmexperimentalhot branch April 1, 2024 04:23
vitalybuka added a commit that referenced this pull request Apr 1, 2024
…k()` (#84851)

Intrinsic declared to have sideeffects, but it's done only to prevent
moving it. Removing unused ones is OK.

Exacted from #84850 for easier review.
vitalybuka added a commit that referenced this pull request Apr 5, 2024
…n}.check()`

Intrinsic introduced with #84850. Intrinsics improves performance
by 3% comparing to removing traps (on
"test-suite/MultiSource/Benchmarks" with PGO+ThinLTO).

The pass will be renamed with #84853.

RFC: https://discourse.llvm.org/t/rfc-add-llvm-experimental-hot-intrinsic-or-llvm-hot/77641

Reviewers: aeubanks, nikic, kstoimenov, dvyukov

Reviewed By: kstoimenov

Pull Request: #84858
vitalybuka added a commit that referenced this pull request Apr 5, 2024
…n}.check()` (#84858)

Intrinsic introduced with #84850. Intrinsics improves performance
by 3% comparing to removing traps (on
"test-suite/MultiSource/Benchmarks" with PGO+ThinLTO).

The pass will be renamed with #84853.

RFC:
https://discourse.llvm.org/t/rfc-add-llvm-experimental-hot-intrinsic-or-llvm-hot/77641
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:ir llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants