[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+      return RValue::get(ArgValue);
+
----------------
erichkeane wrote:
> Do you still need to evaluate this for side-effects even when called with O1?  I think this code only evaluates side-effects in O0 mode at the moment.
Hi, sorry I am not sure about it. Since this part of code is after evaluating all three arguments(including evaluate `ProbArg` with allowing side effect), I think it will evaluate `ProbArg` with side effect whatever the optimization level is. This part of code is just early return the value of first argument `ArgValue` and do not generate code to backend. Do I correctly understand this? Thank you!


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->isValueDependent())
+      return ExprError();
----------------
erichkeane wrote:
> Why is value-dependent an error?  The expression should be able to be a template parameter.  For example:
> 
> 
> struct S {
>     static constexpr float value = 1.1;
> };
> 
> template<typename T>
> void f(bool b) {
>     __builtin_expect_with_probability(b, 1, T::value);
> }
> 
> should work.
> 
> Additionally, in C++20(though not yet implemented in clang it seems):
> 
> template<float F>
> void f(bool b) {
>     __builtin_expect_with_probability(b, 1, F);
> }
> 
> Additionally, how about an integer type?  It would seem that I should be able ot do:
> template<int I>
> void f(bool b) {
>     __builtin_expect_with_probability(b, 1, I); // probability is obviously 0 or 1
> }
> 
Hi, this code is based on Richard's suggestion that checking `ProbArg` is not value-dependent before evaluate. I also saw `EvaluateAsFloat` source code used in CGBuiltin.cpp that it firstly assert the expression is not value-dependent as following:
```
bool Expr::EvaluateAsFloat(APFloat &Result, const ASTContext &Ctx,
                            SideEffectsKind AllowSideEffects,
                            bool InConstantContext) const {
   assert(!isValueDependent() &&
          "Expression evaluator can't be called on a dependent expression.");
```
In fact I am not sure is there anything special should be done when is value-dependent. Do you have suggestions about this? Thank you!


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

https://reviews.llvm.org/D79830