[PATCH] D79830: Add support of __builtin_expect_with_probability

[Erich Keane via Phabricator via llvm-commits]

erichkeane added a comment.

Additionally, it seems you're missing SEMA tests.  Please add those.  Those tests should also make sure that the diagnostics still work with dependent values.



================
Comment at: clang/include/clang/Basic/Builtins.def:567
 BUILTIN(__builtin_expect, "LiLiLi"   , "nc")
+BUILTIN(__builtin_expect_with_probability, "LiLiLid"   , "nc")
 BUILTIN(__builtin_prefetch, "vvC*.", "nc")
----------------
The spaces after the first parameter are odd/unnecessary.  I suspect it is in the line above because it was used to do alignment in the past.  Please don't bother putting them in this new line.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2197
+    const Expr *ProbArg = E->getArg(2);
+    bool EvalSucceed = ProbArg->EvaluateAsFloat(Probability,
+        CGM.getContext(), Expr::SideEffectsKind::SE_AllowSideEffects);
----------------
This is going to give an unused variable warning in release mode, since assert is a macro. You'll have to cast EvalSucceeded to void after the assert.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2199
+        CGM.getContext(), Expr::SideEffectsKind::SE_AllowSideEffects);
+    assert(EvalSucceed == true);
+    llvm::Type *Ty = ConvertType(ProbArg->getType());
----------------
First, don't do ==true here.  Second, add a string to the assert (see other assert uses).


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

https://reviews.llvm.org/D79830