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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
