efriedma 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:
> > 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151587/new/
https://reviews.llvm.org/D151587
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
