[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang marked 15 inline comments as done.
LukeZhuang 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")
----------------
rsmith wrote:
> 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.
Seems there is currently no tag for constant floating-point


================
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
----------------
rsmith wrote:
> LukeZhuang wrote:
> > rsmith wrote:
> > > 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.)
> > Thank you for commenting! I have a question about this. If I check this argument can be fold as constant float in `Sema`, why I need to check it here? Or do you mean I had better create a `ConstantFP` value here after constant emitting? 
> `EmitScalarExpr` will emit arbitrary IR that evaluates the argument. If the argument isn't a simple floating-point literal, then you won't necessarily get an IR-level constant back from that. For example:
> 
> ```
> struct die {
>   constexpr int sides() { return 6; }
>   int roll();
> } d6;
> bool rolled_a_one = __builtin_expect_with_probability(d6.roll(), 1, 1.0 / d6.sides());
> ```
> 
> Here, we can evaluate `1.0 / d6.sides()`, but `EmitScalarExpr` will emit a function call and a division, not a constant.
> 
> Instead, you could use `EvaluateAsFloat` here (you can assert it succeeds because you already checked that in `Sema`) and then directly form a `ConstantFP` from the `APFloat` result.
Fixed in lastest patch


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->EvaluateAsFloat(Confidence, Context)) {
+      double P = Confidence.convertToDouble();
----------------
rsmith wrote:
> LukeZhuang wrote:
> > LukeZhuang wrote:
> > > rsmith wrote:
> > > > 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?
> > > Thank you for commenting. I just read the llvm::Expr document and found that it just have `isIntegerConstantExpr` without a float-point version. Under `EvaluateAsFloat` it says "Return true if this is a constant which we can fold and convert to a floating point value", thus I use this function. Is there any better function to achieve this goal?
> > Hi, and I didn't find other builtin functions ever handle case like this, is there any example I could refer to?
> I think this is the first builtin (and maybe the first part of Clang) that wants to enforce that it sees a floating-point constant expression.
> 
> The right thing to do is generally to call `EvaluateAsConstantExpr` and check that it produces the right kind of value and doesn't generate any notes. See the code at the end of `CheckConvertedConstantExpr` for how to do this.
> 
> You also need to check that `ProbArg` is not value-dependent before you try to evaluate it; it's not meaningful to ask for the value of a value-dependent expression.
fixed in lastest patch


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1806
+    if (ProbArg->EvaluateAsFloat(Confidence, Context)) {
+      double P = Confidence.convertToDouble();
+      if (P < 0.0 || P > 1.0) {
----------------
rsmith wrote:
> 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?
fixed in lastest patch


================
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)
----------------
rsmith wrote:
> This does not disallow NaN values. I think you want `if (!(P >= 0.0 && P <= 1.0))` or equivalent instead.
fixed in lastest patch


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1813-1814
+    } else {
+      Diag(ProbArg->getLocStart(), diag::err_probability_not_constant_float)
+          << ProbArg->getSourceRange();
+      return ExprError();
----------------
rsmith wrote:
> `EvaluateAsConstantExpr` will give you some notes to attach to this diagnostic to explain why the expression is non-constant.
fixed in lastest patch


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

https://reviews.llvm.org/D79830