[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang marked 12 inline comments as done.
LukeZhuang added a comment.

In D79830#2075038 <https://reviews.llvm.org/D79830#2075038>, @davidxl wrote:

> Can you add some tests at the LLVM side?


added ll test case



================
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)
----------------
LukeZhuang wrote:
> 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.
Sorry, after I test with different targets I understood your suggestion. It shows weird behavior in comparing for AVR target. Now I have converted to IEEEdouble. I also added test for AVR target and passed it.


================
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]}}
----------------
rsmith wrote:
> 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.
added more edge cases here and example for AVR target


================
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.
 
----------------
rsmith wrote:
> 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.
I also think I might had better add intrinsic documents in llvm/docs/LangRef.rst instead of here, and keep __builtin_exepct_with_probability here or discuss with the original author about this.


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

https://reviews.llvm.org/D79830