[PATCH] D79830: Add support of __builtin_expect_with_probability

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

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/Builtins.def:567
 BUILTIN(__builtin_expect, "LiLiLi"   , "nc")
+BUILTIN(__builtin_expect_with_probability, "LiLiLid"   , "nc")
 BUILTIN(__builtin_prefetch, "vvC*.", "nc")
----------------
I assume we don't have any equivalent of the `I` flag to require a floating-point constant expression? If not, it's probably not worth adding that if this builtin would be the only user of it.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2195
+    Value *ExpectedValue = EmitScalarExpr(E->getArg(1));
+    Value *Confidence = EmitScalarExpr(E->getArg(2));
+    // Don't generate llvm.expect.with.probability on -O0 as the backend
----------------
If the intrinsic expects a `ConstantFP` value, this isn't enough to guarantee that you get one (the set of cases that we fold to constants during expression emission is much smaller than the set of cases we can constant-evaluate). You should run the constant evaluator first, to produce an `APFloat`, then emit that value as a constant. (Optionally you can form a `ConstantExpr` as part of the check in `Sema` and store the value in that object rather than recomputing it here.)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->EvaluateAsFloat(Confidence, Context)) {
+      double P = Confidence.convertToDouble();
----------------
What rules should be applied here? Do we just want to allow anything that we can evaluate (which might change over time), or do we want to use the underlying language's notion of floating-point constant expressions, if it has them?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1806
+    if (ProbArg->EvaluateAsFloat(Confidence, Context)) {
+      double P = Confidence.convertToDouble();
+      if (P < 0.0 || P > 1.0) {
----------------
Converting to a host `double` here seems suspicious and unnecessary: can you form a suitable `APFloat` representation of `1.0` and `0.0` instead, and compare to those?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1807
+      double P = Confidence.convertToDouble();
+      if (P < 0.0 || P > 1.0) {
+        Diag(ProbArg->getLocStart(), diag::err_probability_out_of_range)
----------------
This does not disallow NaN values. I think you want `if (!(P >= 0.0 && P <= 1.0))` or equivalent instead.


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

https://reviews.llvm.org/D79830