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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
