rsmith added a comment.
Rather than adding a `CanDelayEvaluation` flag to call expressions, I think it
would be substantially better to introduce a new `Expr` wrapper node for
expressions that are required to be constant expressions. (That'd eventually
also give us a place to cache the evaluated value of such an expression, where
today we recompute it each time it's needed.) Essentially, you would add a
`ConstExpr` node (or similar), teach `IgnoreImplicit` and friends to step over
it, and add it in the places where we semantically require an expression to be
a constant expression. This has various benefits: there are other upcoming
language features (in C++20) that require this knowledge and don't necessarily
have a `CallExpr` to tie it to, and it gives us a place to stash an evaluated
value, and it gives IR generation a simple way to detect expressions that it
can constant-evaluate rather than emitting.
When the evaluator steps into such an expression, it can track that it's done
so, and ensure that it always produces a constant value for any
`__builtin_constant_p` calls in that scope.
================
Comment at: include/clang/AST/Expr.h:2290
SourceLocation RParenLoc;
+ bool CanDelayEvaluation;
----------------
It's not reasonable to make all `CallExpr`s `sizeof(void*)` bytes larger for
this. If we really need this, you can track it it in the `CallExprBits` on the
base class. (But you'll also need to extend at least the Serialization and
ASTImporter code to handle it, and probably TreeTransform too.)
But I don't think you need this.
================
Comment at: lib/AST/ExprConstant.cpp:8162
+ return Success(true, E);
+ if (isa<DeclRefExpr>(Arg->IgnoreParenCasts()) &&
+ E->getCanDelayEvaluation())
----------------
rsmith wrote:
> Your `canDelayEvaluation` check does not appear to cover several of the cases
> we'd need to check for here. Eg:
>
> ```
> void f(int n) {
> enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because
> a's value is non-constant
> ```
>
>
Hmm, OK. Per https://bugs.llvm.org/show_bug.cgi?id=4898#c38, the correct check
would be whether the reason we found the expression to be non-constant was that
it tried to read the value of a local variable or function parameter. Eg, for:
```
void f(int x, int y) {
bool k = __builtin_constant_p(3 + x < 5 * y);
```
... we should defer the `__builtin_constant_p` until after optimization. (And
we should never do this if the expression has side-effects.) But given that
your goal is merely to improve the status quo, not to exactly match GCC, this
may be sufficient.
You should at least check that the variable is non-volatile, though. (More
generally, check that `Arg` doesn't have side-effects.)
================
Comment at: lib/AST/ExprConstant.cpp:8162-8164
+ if (isa<DeclRefExpr>(Arg->IgnoreParenCasts()) &&
+ E->getCanDelayEvaluation())
+ return false;
----------------
Your `canDelayEvaluation` check does not appear to cover several of the cases
we'd need to check for here. Eg:
```
void f(int n) {
enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because
a's value is non-constant
```
================
Comment at: lib/AST/ExprConstant.cpp:8164
+ E->getCanDelayEvaluation())
+ return false;
+ return Success(false, E);
----------------
It's not correct to return `false` here without producing a diagnostic
explaining why the evaluation is non-constant. You can use `Info.FFDiag()` to
produce a default "invalid subexpression" diagnostic, which is probably
sufficient given that we won't actually show the diagnostic to the user in most
cases.
================
Comment at: lib/Sema/SemaExpr.cpp:5694
+ MarkBuiltinConstantPCannotDelayEvaluation(InitExpr);
return BuildCompoundLiteralExpr(LParenLoc, TInfo, RParenLoc, InitExpr);
----------------
Why are you marking this? This isn't (necessarily) a context where a constant
expression is required. (In C, the overall initializer for a global would be,
but a local-scope compound literal would not.)
Also, this should be in the `Build` function, not in the `ActOn` function.
================
Comment at: lib/Sema/SemaExpr.cpp:5799
E->setType(Context.VoidTy); // FIXME: just a place holder for now.
+ MarkBuiltinConstantPCannotDelayEvaluation(E);
return E;
----------------
Likewise here, I don't see the justification for this.
https://reviews.llvm.org/D52854
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
