[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang marked 6 inline comments as done.
LukeZhuang added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->isValueDependent())
+      return ExprError();
----------------
erichkeane wrote:
> 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.
Hi, is that mean I just not not need this check, and then add test case as you suggest to check it does not yield error, is that correct? Thank you!


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1812
+                                          Context)) ||
+        !Eval.Val.isFloat()) {
+      Diag(ProbArg->getLocStart(), diag::err_probability_not_constant_float)
----------------
erichkeane wrote:
> BTW, this check should be valid (ensuring its a float), since normal conversions should happen as a part of the function call.
Do you mean that when customer pass an integer here and make sure this can handle? I just test this and find `isFloat()` returns true when this argument is `constexpr int`. Seems it is automatically converted here.


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

https://reviews.llvm.org/D79830