[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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