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