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