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