[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang added inline comments.


================
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)
----------------
rsmith wrote:
> 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`.
Hi, thank you for comments! I have several questions here:
(1) I do not understand why "APFloat Probability >= APFloat(0.0)" is affected by target here. Is this just a double come from C front-end and I check it here? 
(2) And also not sure in CGBuiltin, is there I convert it to llvm intrinsic? How does target double type affect it?
(3) If I convert APFloat to IEEEDouble, should I use APF.convertToDouble or create a DoubleAPFloat. My original code did APFloat.convertToDouble and compare, and I do not sure about the difference.


================
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.
----------------
rsmith wrote:
> 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.
Yeah, thank you! I think your suggestion makes much sense, it should be in clang. However, the original __builtin_expect part is not written by me and and I am not sure its affect. So I added the author of this part as reviewer and I think it had better to ask his idea about this.


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

https://reviews.llvm.org/D79830