[PATCH] D79830: Add support of __builtin_expect_with_probability

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

rsmith added inline comments.


================
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
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->EvaluateAsFloat(Confidence, Context)) {
+      double P = Confidence.convertToDouble();
----------------
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.


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


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

https://reviews.llvm.org/D79830