[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

Mon Jun 5 11:21:17 PDT 2023

nickdesaulniers added inline comments.

================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
     // This is a string literal initializing an array in an initializer.
-    return CGM.GetConstantArrayFromStringLiteral(E);
+    return E->isLValue() ?
+      CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :
----------------
efriedma wrote:
> efriedma wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > nickdesaulniers wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > efriedma wrote:
> > > > > > > > nickdesaulniers wrote:
> > > > > > > > > efriedma wrote:
> > > > > > > > > > Maybe we should have a separate ConstExprEmitterLValue... trying to handle both LValues and RValues on the same codepath has been problematic in the past.  It's very easy for code to get confused what it's actually trying to emit.
> > > > > > > > > So we'd have a `ConstExprEmitterLValue` class with some visitor methods, and a `ConstExprEmitterRValue` with other methods implemented?
> > > > > > > > Something like that.
> > > > > > > > 
> > > > > > > > Actually thinking about it a bit more, not sure you need to actually implement ConstExprEmitterLValue for now.  You might just be able to ensure we don't ever call ConstExprEmitter with an lvalue.  The current ConstExprEmitter doesn't expect lvalues, and shouldn't call itself with lvalues.  (We bail on explicit LValueToRValue conversions.)  And Evaluate() shouldn't actually evaluate the contents of an lvalue if it isn't dereferenced, so there hopefully aren't any performance issues using that codepath.
> > > > > > > > 
> > > > > > > > In terms of implementation, I guess that's basically restoring the destType->isReferenceType() that got removed?  (I know I suggested it, but I wasn't really thinking about it...)
> > > > > > > One thing I think we may need to add to `ConstExprEmitter` is the ability to evaluate `CallExpr`s based on certain test case failures...does that seem right?
> > > > > > See also the calls to `constexpr f()` in clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > > > > The only things I know of that Evaluate() can't handle are CK_ToUnion, CK_ReinterpretMemberPointer, and DesignatedInitUpdateExpr.  DesignatedInitUpdateExpr is related to the failures in test/CodeGenCXX/designated-init.cpp; I don't think the others show up in any of the testcases you've mentioned.  (CK_ToUnion can't appear in C++ code. CK_ReinterpretMemberPointer is a `reinterpret_cast<T>` where T is a member pointer type.)
> > > > > 
> > > > > Given none of those constructs show up in const-init-cxx1y.cpp, the only reason for it to fail is if we aren't correctly falling back for a construct the current code doesn't know how to handle.  You shouldn't need to implement any new constructs.
> > > > in clang/test/CodeGenCXX/designated-init.cpp we have:
> > > > ```
> > > > >> 22 namespace ModifyStaticTemporary {                                               
> > > >    23   struct A { int &&temporary; int x; };                                         
> > > >    24   constexpr int f(int &r) { r *= 9; return r - 12; }                            
> > > >    25   A a = { 6, f(a.temporary) };
> > > > ```
> > > > In the AST, that looks like:
> > > > ```
> > > > | |-VarDecl 0x562b77df39b0 <line:25:3, col:29> col:5 used a 'A':'ModifyStaticTemporary::A' cinit
> > > > | | `-ExprWithCleanups 0x562b77df3c68 <col:9, col:29> 'A':'ModifyStaticTemporary::A'
> > > > | |   `-InitListExpr 0x562b77df3bb8 <col:9, col:29> 'A':'ModifyStaticTemporary::A'
> > > > | |     |-MaterializeTemporaryExpr 0x562b77df3c08 <col:11> 'int' xvalue extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
> > > > | |     | `-IntegerLiteral 0x562b77df3a18 <col:11> 'int' 6
> > > > | |     `-CallExpr 0x562b77df3b30 <col:14, col:27> 'int'
> > > > | |       |-ImplicitCastExpr 0x562b77df3b18 <col:14> 'int (*)(int &)' <FunctionToPointerDecay>
> > > > | |       | `-DeclRefExpr 0x562b77df3ad0 <col:14> 'int (int &)' lvalue Function 0x562b77df37a0 'f' 'int (int &)'
> > > > | |       `-MemberExpr 0x562b77df3aa0 <col:16, col:18> 'int' lvalue .temporary 0x562b77df35c0
> > > > | |         `-DeclRefExpr 0x562b77df3a80 <col:16> 'A':'ModifyStaticTemporary::A' lvalue Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
> > > > ```
> > > > (So, indeed no `DesignatedInitUpdateExpr`) but the call to the `constexpr` `f()` updates the reference (to `54`).  If I remove the visitor for `MaterializeTemporaryExpr`, we fail to evaluate `f` and end up emitting `6` rather than `54`.  Doesn't that mean that the fast path (`ConstExprEmitter`) needs to be able to evaluate `CallExpr`?
> > > > 
> > > > Or should `VisitInitListExpr` bail if any of the inits `isa<MaterializeTemporaryExpr>` (or perhaps `isa<CallExpr>`)?
> > > There are a few related cases here.
> > > 
> > > Case number one is when you have something like `int z(); A a = { z(), z() };`.  There's no constant evaluation going on: you just emit two zero-initialized variables, and the runtime init initializes both of them.
> > > 
> > > Case number two is when everything is obviously constant: something like `A a = { 1, 2 };`
> > > 
> > > Case number three is when there are simple side-effects, and the standard requires we evaluate them at compile-time.  Something like `A a = { 1, ++a.temporary };`.  In this case, we need to ensure that we use Evaluate() to compute the value of both the temporary and the variable.  The literal "1" is not the correct value to use.  CodeGenModule::GetAddrOfGlobalTemporary is supposed to ensure we use the value from the evaluation of the variable as a whole (see comment "If the initializer of the extending declaration").
> > > 
> > > Case number four is when we can't constant-evaluate a variable as a whole, but we do evaluate some of the temporaries involved.  Something like `int z(); A a = { 1, a.temporary += z() };`  In this case, we constant-evaluate the temporary using the initial value, then emit runtime initialization to finish computing the value of the variable as a whole.
> > > 
> > > You example should fall under case three.  Both the temporary and the variable should be evaluated by Evaluate().
> > > 
> > > I'm not sure how the code ends up emitting the value 6, but hopefully that helps?
> > Oh, I think I see what's happening; the code that looks for the temporary in GetAddrOfGlobalTemporary isn't reliable if the whole variable isn't evaluated first.  It ends up pulling out the result of a partial evaluation, or something like that.
> > 
> > Making EmitArrayInitialization/EmitRecordInitialization bail if they see a MaterializeTemporaryExpr should deal with the issue, I think?  Not sure if you'd need to recursively visit all the initializers (I don't remember what constructs allow lifetime extension off the top of my head).
> To be more precise, what happens is that calling EvaluateAsLValue on the MaterializeTemporaryExpr actually *corrupts* the computed value of the temporary: the complete variable is evaluated earlier for other reasons, then EvaluateAsLValue overwrites the correct value we computed earlier with the wrong value.
> In this case, we need to ensure that we use Evaluate() to compute the value of both the temporary and the variable.

Just triple checking, `Evaluate` is the "slow path" (i.e. `VarDecl::evaluateValue`, `Expr::EvaluateAsLValue`, and `Expr::EvaluateAsRValue`?

> To be more precise, what happens is that calling EvaluateAsLValue on the MaterializeTemporaryExpr actually *corrupts* the computed value of the temporary

So the slow path gets it wrong? But prior to this patch, that's was used first before ConstExprEmitter?  (Maybe I should add more comments about fast vs slow path)

---

> Not sure if you'd need to recursively visit all the initializers (I don't remember what constructs allow lifetime extension off the top of my head).

I think I would; the last test case currently failing is clang/test/CodeGenCXX/atomicinit.cpp:
```
struct X {
  constexpr X(int n) : n(n) {}
  short n;
  char c = 6;
};

struct Y {
  _Atomic(X) a;
  _Atomic(int) b;
};
Y y = { X(4), 5 };
```
The AST for `y` looks like:
```
`-VarDecl 0x562bba0cad00 <line:11:1, col:17> col:3 y 'Y':'Y' cinit
  `-ExprWithCleanups 0x562bba0e9f28 <col:7, col:17> 'Y':'Y'
    `-InitListExpr 0x562bba0e9c20 <col:7, col:17> 'Y':'Y'
      |-ImplicitCastExpr 0x562bba0e9ef8 <col:9, col:12> '_Atomic(X)' <NonAtomicToAtomic>
      | `-ImplicitCastExpr 0x562bba0e9ee0 <col:9, col:12> 'X':'X' <ConstructorConversion>
      |   `-CXXConstructExpr 0x562bba0e9eb0 <col:9, col:12> 'X':'X' 'void (X &&) noexcept' elidable
      |     `-MaterializeTemporaryExpr 0x562bba0e9c70 <col:9, col:12> 'X':'X' xvalue
      |       `-CXXFunctionalCastExpr 0x562bba0e9b88 <col:9, col:12> 'X':'X' functional cast to X <ConstructorConversion>
      |         `-CXXConstructExpr 0x562bba0e9a10 <col:9, col:12> 'X':'X' 'void (int)'
      |           `-IntegerLiteral 0x562bba0cadc0 <col:11> 'int' 4
      `-ImplicitCastExpr 0x562bba0e9f10 <col:15> '_Atomic(int)' <NonAtomicToAtomic>
        `-IntegerLiteral 0x562bba0e9bb0 <col:15> 'int' 5
```
so we'd need to peek through the casts to find that there was a `MaterializeTemporaryExpr` in there, then bail?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587