Tyker added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:6807-6808
+ llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
return StmtVisitorTy::Visit(E->getSubExpr());
}
----------------
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.
================
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();
----------------
rsmith wrote:
> 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.
this can be relaxed to hasAPValueResult without any change in behavior.
================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1374
+ emitAbstract(CE->getBeginLoc(), CE->getAPValueResult(), RetType);
+ return Res;
+}
----------------
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);
```
================
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())));
+ }
----------------
rsmith wrote:
> 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.
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.
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
