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

[Tyker via Phabricator via cfe-commits]

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