[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang marked 4 inline comments as done.
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)
----------------
lebedev.ri wrote:
> This doesn't look right to me, but maybe i'm misreading something?
> Assuming the `!` is intentional to avoid some fp weirdness (if not, simplify this)
> shouldn't this be an `&&`, not `||`?
> 
> Consider:
> `Probability = -1`
> ```
>     if (!(-1 >= llvm::APFloat(0.0) ||
>           -1 <= llvm::APFloat(1.0))) {
> ```
> ```
>     if (!(false ||
>           true) {
> ```
> ```
>     if (false) {
> ```
> so i'd think `Probability = -1` would not get diagnosed?
Yes, you're right.. Because of version problem, I still use converting to double in my local build, and miscopy this when upstreaming. Thank you for point it out.


================
Comment at: clang/test/Sema/builtin-expect-with-probability.cpp:19-20
+  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]}}
+  dummy = __builtin_expect_with_probability(x > 0, 1, -1); // expected-error {{probability of __builtin_expect_with_probability is outside the range [0.0, 1.0]}}
+  dummy = __builtin_expect_with_probability(x > 0, 1, p); // expected-error {{probability of __builtin_expect_with_probability must be constant floating-point expression}} expected-note {{read of non-constexpr variable 'p' is not allowed in a constant expression}}
----------------
lebedev.ri wrote:
> Do these tests actually pass?
It pass it for now. Again my local build still use converting to double to compare and passed the test, but I miscopied it when upstream the change. Thank you!


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

https://reviews.llvm.org/D79830