[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:2202
+    // won't use it for anything.
+    // Note, we still IRGen ExpectedValue because it could have side-effects.
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0)
----------------
erichkeane wrote:
> I believe in EvaluateAsFloat you need to pass  'allow side effects', because by default it does not.
Thinking further, Since it is with constant-expressions, we might consider disallowing side effects.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+      return RValue::get(ArgValue);
+
----------------
LukeZhuang wrote:
> 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!
You're right here, I'd read the comment and skipped that ArgValue was evaluated above.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->isValueDependent())
+      return ExprError();
----------------
LukeZhuang wrote:
> 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!
Typically in dependent cases (though this file doesn't deal with the much), we just presume the arguments are valid. The values then get checked when instantiated.  Tests to show this would also be necessary.


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

https://reviews.llvm.org/D79830