[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via llvm-commits]

LukeZhuang 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
----------------
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? 


================
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:
> 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?


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

https://reviews.llvm.org/D79830