rsmith added inline comments.
================
Comment at: clang/include/clang/AST/Expr.h:1062
+ return ConstantExprBits.APValueKind != APValue::None &&
+ ConstantExprBits.APValueKind != APValue::Indeterminate;
}
----------------
Why do we need to treat indeterminate values specially here? Are we using that
value inappropriately in `ExprConstant`?
================
Comment at: clang/lib/AST/ExprConstant.cpp:6807-6808
+ llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
return StmtVisitorTy::Visit(E->getSubExpr());
}
----------------
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.
================
Comment at: clang/lib/AST/ExprConstant.cpp:10772-10777
bool IntExprEvaluator::VisitConstantExpr(const ConstantExpr *E) {
llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
- if (E->getResultAPValueKind() != APValue::None)
+ if (E->hasAPValueResult())
return Success(E->getAPValueResult(), E);
return ExprEvaluatorBaseTy::VisitConstantExpr(E);
}
----------------
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?
================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1364-1365
+llvm::Constant *ConstantEmitter::tryEmitConstantExpr(const ConstantExpr *CE) {
+ if (!CE->isImmediateInvocation())
+ return nullptr;
+ const Expr *Inner = CE->getSubExpr()->IgnoreImplicit();
----------------
I'm fine with having this check for now, but eventually I think we should do
this for all `ConstantExpr`s, regardless of whether they're immediate
invocations.
================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1374
+ emitAbstract(CE->getBeginLoc(), CE->getAPValueResult(), RetType);
+ return Res;
+}
----------------
Can we assert that we succeeded here? This emission should never fail.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:464-470
+ QualType RetType = cast<CallExpr>(E->getSubExpr()->IgnoreImplicit())
+ ->getCallReturnType(CGF.getContext());
+ if (RetType->isReferenceType()) {
+ return CGF.Builder.CreateLoad(Address(
+ Result, CGF.getContext().getTypeAlignInChars(
+ cast<ReferenceType>(RetType)->getPointeeType())));
+ }
----------------
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.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:1095
+ if (const auto *EWC = dyn_cast_or_null<ExprWithCleanups>(RV)) {
+ enterFullExpression(EWC);
+ RV = EWC->getSubExpr();
----------------
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*`?
================
Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:1
-// RUN: %clang_cc1 -std=c++2a -fsyntax-only -Wno-unused-value %s -verify
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -Wno-unused-value %s -verify >
/dev/null
----------------
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.
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
