with this patch instead of emitting calls to consteval function. the IR-gen will emit a store of the already computed result.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/AST/Expr.h | ||
---|---|---|
1062 ↗ | (On Diff #264023) | Why do we need to treat indeterminate values specially here? Are we using that value inappropriately in ExprConstant? |
clang/lib/AST/ExprConstant.cpp | ||
6807–6808 | 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:
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. | |
10769–10770 | This override looks equivalent to the base class version. (The only difference appears to be whether IsConstantContext is set during the call to Success, but I don't think Success cares about that flag.) Can you remove this override? | |
clang/lib/CodeGen/CGExprConstant.cpp | ||
1364–1365 | I'm fine with having this check for now, but eventually I think we should do this for all ConstantExprs, regardless of whether they're immediate invocations. | |
1374 | Can we assert that we succeeded here? This emission should never fail. | |
clang/lib/CodeGen/CGExprScalar.cpp | ||
423–429 | OK, so this is presumably handling the case where ScalarExprEmitter is used to emit an lvalue expression, under the understanding that when it reaches the eventual lvalue a load will be implicitly generated. Looking for a CallExpr that returns a reference type is not the best way to handle this. It's brittle (it would break if tryEmitConstantExpr starts emitting more kinds of ConstantExpr or if we start supporting more kinds of immediate invocations) and we don't need to perform such a subtle check: instead, please just check whether E is an lvalue, and perform a load if so. | |
clang/lib/CodeGen/CGStmt.cpp | ||
1122 | Presumably it's OK to skip this for ConstantExpr because by definition the ConstantExpr node isn't tracking any block cleanups. If that's the case, should we rename this to enterExprWithCleanups and change it to take an ExprWithCleanups* rather than a FullExpr*? | |
clang/test/SemaCXX/cxx2a-consteval.cpp | ||
1 | Use -emit-llvm-only instead of -emit-llvm > /dev/null if you want to make sure that emission succeeds but don't want to check the IR. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
6807–6808 | ok seems reasonable. | |
clang/lib/CodeGen/CGExprConstant.cpp | ||
1364–1365 | this can be relaxed to hasAPValueResult without any change in behavior. | |
1374 | 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); | |
clang/lib/CodeGen/CGExprScalar.cpp | ||
423–429 | we need to generate a load for rvalue-reference as well so i think we need to check wether E is a glvalue instead of just lvalue. |
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.
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
6807–6808 | 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. | |
clang/lib/CodeGen/CGExprConstant.cpp | ||
1374 | 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. |
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:
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:
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.