[PATCH] D79830: Add support of __builtin_expect_with_probability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

FYI: I'm more of a clang contributor, so I'm unable to review the LLVM code, hopefully someone will come along who can check on that.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+    const Expr *ProbArg = TheCall->getArg(2);
+    if (ProbArg->isValueDependent())
+      return ExprError();
----------------
LukeZhuang wrote:
> 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!
Right, exactly.  This code path should end up being evaluated 2x in the case of an instantiation, the first time it is with the dependent placeholder types.  The second time it will be instantiated.  SO if you cannot validate the rules against dependent values, you just assume it would work and let the 2nd pass catch it.

Include some tests to make sure you catch this right and that this is usable in templates.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1812
+                                          Context)) ||
+        !Eval.Val.isFloat()) {
+      Diag(ProbArg->getLocStart(), diag::err_probability_not_constant_float)
----------------
LukeZhuang wrote:
> 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.
Ok, thanks for checking. I was unsure, but I presumed that the implicit type-conversion based on the builtin definition would have caused that to happen.  


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

https://reviews.llvm.org/D79830