[PATCH] D79830: Add support of __builtin_expect_with_probability

[Erich Keane via Phabricator via llvm-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2197
+    const Expr *ProbArg = E->getArg(2);
+    ProbArg->EvaluateAsFloat(Probability, CGM.getContext());
+    llvm::Type *Ty = ConvertType(ProbArg->getType());
----------------
You likely need to assert on the return value (as Richard suggested).


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2202
+    // won't use it for anything.
+    // Note, we still IRGen ExpectedValue because it could have side-effects.
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0)
----------------
I believe in EvaluateAsFloat you need to pass  'allow side effects', because by default it does not.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+      return RValue::get(ArgValue);
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->isValueDependent())
+      return ExprError();
----------------
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
}



================
Comment at: clang/lib/Sema/SemaChecking.cpp:1812
+                                          Context)) ||
+        !Eval.Val.isFloat()) {
+      Diag(ProbArg->getLocStart(), diag::err_probability_not_constant_float)
----------------
BTW, this check should be valid (ensuring its a float), since normal conversions should happen as a part of the function call.


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

https://reviews.llvm.org/D79830