[PATCH] D76420: Prevent IR-gen from emitting consteval declarations

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

rsmith accepted this revision.
rsmith marked an inline comment as done.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks! Looks good.

I'd like to eventually get to a point where every `ConstantExpr` that reaches code generation has `hasAPValueResult()` return `true`, but that doesn't need to be done now.



================
Comment at: clang/lib/AST/ExprConstant.cpp:6807-6808
 
+    llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
     return StmtVisitorTy::Visit(E->getSubExpr());
   }
----------------
Tyker wrote:
> rsmith wrote:
> > I don't think this is really right, but perhaps the difference isn't observable right now. What I'm thinking of is a case like this:
> > 
> > ```
> > consteval int do_stuff() {
> >   __builtin_produce_diagnostic("hello world\n");
> >   return 42;
> > }
> > constexpr int f() {
> >   int n = do_stuff();
> >   return n;
> > }
> > int k = f();
> > ```
> > 
> > Here, I would expect that when we process the immediate invocation of `do_stuff()` in `f`, we would immediately evaluate it, including issuing its diagnostic. And then for all subsequent calls to `f()`, we would never re-evaluate it.
> > 
> > I can see a couple of ways this could work:
> > 
> >  * Whenever we create a `ConstantExpr`, we always evaluate it and fill in the `APValue` result; it's never absent except perhaps in a window of time between forming that AST node and deciding for sure that we want to keep it (for nested immediate invocation handling).
> >  * Whenever constant evaluation meets a `ConstantExpr` that doesn't have an associated result yet, it triggers that result to be computed and cached, as a separate evaluation.
> > 
> > I think the first of those two approaches is much better: lazily evaluating the `ConstantExpr` will require us to save update records if we're writing an AST file, and will mean we don't always have an obvious point where the side-effects from builtin consteval functions (eg, reflection-driven actions) happen.
> > 
> > So I think the right thing to do is probably to say that a `ConstantExpr` that hasn't yet had its `APValue` result filled in is non-constant for now, and to ensure that everywhere that creates a `ConstantExpr` always eventually either removes it again or fills in the result.
> ok seems reasonable.
> 
OK, so we're still evaluating through a not-yet-evaluated immediate invocation. This is subtle, but I think it's correct: when this happens, either we're in some enclosing immediate invocation, in which case we want to perform any side-effects from inside this `ConstantExpr`, or we're not, in which case those side-effects should be prohibited anyway.


================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1374
+      emitAbstract(CE->getBeginLoc(), CE->getAPValueResult(), RetType);
+  return Res;
+}
----------------
Tyker wrote:
> rsmith wrote:
> > Can we assert that we succeeded here? This emission should never fail.
> from the comment on emitAbstract's declaration it seems to already be the case.
> ```
>   /// Emit the result of the given expression as an abstract constant,
>   /// asserting that it succeeded.  This is only safe to do when the
>   /// expression is known to be a constant expression with either a fairly
>   /// simple type or a known simple form.
>   llvm::Constant *emitAbstract(const Expr *E, QualType T);
>   llvm::Constant *emitAbstract(SourceLocation loc, const APValue &value,
>                                QualType T);
> ``` 
Given that this function can return `nullptr` on other control paths, I think it would be useful (as documentation of the intent) to `assert(Res);` here, even if `emitAbstract` already guarantees that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76420