[PATCH] D79830: Add support of __builtin_expect_with_probability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10804-10807
+   "probability of __builtin_expect_with_probability must be constant "
+   "floating-point expression">;
+def err_probability_out_of_range : Error<
+   "probability of __builtin_expect_with_probability is outside the "
----------------
I think "probability argument to" rather than "probability of" would be clearer.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2202
+    // won't use it for anything.
+    // Note, we still IRGen ExpectedValue because it could have side-effects.
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0)
----------------
erichkeane wrote:
> erichkeane wrote:
> > I believe in EvaluateAsFloat you need to pass  'allow side effects', because by default it does not.
> Thinking further, Since it is with constant-expressions, we might consider disallowing side effects.
(This is marked as done, but `SE_AllowSideEffects` is still passed above.)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1818-1819
+    llvm::APFloat Probability = Eval.Val.getFloat();
+    if (!(Probability >= llvm::APFloat(0.0) &&
+          Probability <= llvm::APFloat(1.0))) {
+      Diag(ProbArg->getLocStart(), diag::err_probability_out_of_range)
----------------
I think this will probably only work if the target `double` type is `IEEEdouble`. (Otherwise you'll be comparing an `IEEEdouble` `APFloat` to a different kind of `APFloat`.)

We do not guarantee that `double` is `IEEEdouble` for all our supported targets. In particular, for TCE and for AVR (at least under `-mdouble=32`), `double` is `IEEEsingle` instead.

It looks like your LLVM intrinsic expects an LLVM IR `double` (that is, an `IEEEdouble`) as its third argument, so perhaps the right thing to do would be to convert the `APFloat` to `IEEEdouble` here and in CodeGen. Please add a test for a target where `double` is not `IEEEdouble`.


================
Comment at: clang/test/Sema/builtin-expect-with-probability.cpp:18
+  bool dummy;
+  dummy = __builtin_expect_with_probability(x > 0, 1, 0.9);
+  dummy = __builtin_expect_with_probability(x > 0, 1, 1.1); // expected-error {{probability of __builtin_expect_with_probability is outside the range [0.0, 1.0]}}
----------------
Please also test some more edge cases. such as: if the probability argument is not convertible to float, or is inf or nan, negative zero, -denorm_min, or 1+epsilon.


================
Comment at: llvm/docs/BranchWeightMetadata.rst:18
 Branch weights might be fetch from the profiling file, or generated based on
-`__builtin_expect`_ instruction.
+`__builtin_expect`_ and `__builtin_expect_with_probability`_ instruction.
 
----------------
There are no instructions with these names. There are Clang builtin functions with these names and LLVM intrinsics with similar but different names. This document should be documenting the `llvm.expect` and `llvm.expect.with.probability` intrinsics.


================
Comment at: llvm/docs/BranchWeightMetadata.rst:128-135
+Built-in ``expect.with.probability`` Instruction
+================================================
+
+``__builtin_expect_with_probability(long exp, long c, double probability)`` has
+the same semantics as ``__builtin_expect``, but the caller provides the
+probability that ``exp == c``. The last argument ``probability`` must be
+constant floating-point expression and be in the range [0.0, 1.0] inclusive.
----------------
This whole section of documentation (line 86 to 166) seems inappropriate to me as LLVM documentation, because it's talking about the behavior of one particular source language with one particular frontend, not LLVM semantics. Here's what I'd suggest:

 * Move all of that documentation into the Clang user's manual, as a description of the `__builtin_expect*` builtin functions.
 * Here in the LLVM documentation, add documentation for the `llvm.expect` and `llvm.expect.with.probability` intrinsic functions, and include a note pointing to the Clang documentation and suggesting that `__builtin_expect` and `__builtin_expect_with_probability` can be used to generate these intrinsics in C and C++ source files.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79830/new/

https://reviews.llvm.org/D79830