efriedma added inline comments.
================ Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa<MaterializeTemporaryExpr>(E)) + return nullptr; + ---------------- nickdesaulniers wrote: > efriedma wrote: > > nickdesaulniers wrote: > > > efriedma wrote: > > > > efriedma wrote: > > > > > This needs a comment explaining why we're bailing out here. > > > > We might need to do a recursive visit still, to handle the cases noted > > > > at > > > > https://en.cppreference.com/w/cpp/language/reference_initialization#Lifetime_of_a_temporary > > > > . Not constructors, but other things. I think we don't have existing > > > > testcases, but for example `typedef int x[2]; struct Z { int &&x, y; }; > > > > Z z = { x{1,2}[0], z.x=10 };` > > > The AST for that test case: > > > ``` > > > `-VarDecl 0x55809e1c6110 <col:45, col:71> col:47 used z 'Z':'Z' cinit > > > `-ExprWithCleanups 0x55809e1c6658 <col:51, col:71> 'Z':'Z' > > > `-InitListExpr 0x55809e1c64c8 <col:51, col:71> 'Z':'Z' > > > |-ArraySubscriptExpr 0x55809e1c63b8 <col:53, col:61> 'int' xvalue > > > | |-ImplicitCastExpr 0x55809e1c63a0 <col:53, col:58> 'int *' > > > <ArrayToPointerDecay> > > > | | `-MaterializeTemporaryExpr 0x55809e1c6388 <col:53, col:58> > > > 'x':'int[2]' xvalue extended by Var 0x55809e1c6110 'z' 'Z':'Z' > > > | | `-CXXFunctionalCastExpr 0x55809e1c6310 <col:53, col:58> > > > 'x':'int[2]' functional cast to x <NoOp> > > > | | `-InitListExpr 0x55809e1c62c0 <col:54, col:58> 'x':'int[2]' > > > | | |-IntegerLiteral 0x55809e1c6230 <col:55> 'int' 1 > > > | | `-IntegerLiteral 0x55809e1c6250 <col:57> 'int' 2 > > > | `-IntegerLiteral 0x55809e1c6338 <col:60> 'int' 0 > > > `-ImplicitCastExpr 0x55809e1c6560 <col:64, col:68> 'int' > > > <LValueToRValue> > > > `-BinaryOperator 0x55809e1c6448 <col:64, col:68> 'int' lvalue '=' > > > |-MemberExpr 0x55809e1c63f8 <col:64, col:66> 'int' lvalue .x > > > 0x55809e1c5fe0 > > > | `-DeclRefExpr 0x55809e1c63d8 <col:64> 'Z':'Z' lvalue Var > > > 0x55809e1c6110 'z' 'Z':'Z' > > > `-IntegerLiteral 0x55809e1c6428 <col:68> 'int' 10 > > > ``` > > > my code at this revision `Diff 529732` (without recursive visitation) > > > produces: > > > ``` > > > @_ZGR1z_ = internal global [2 x i32] [i32 1, i32 2], align 4 > > > @z = dso_local global { ptr, i32 } { ptr @_ZGR1z_, i32 10 }, align 8 > > > ``` > > > so yeah, it looks wrong and differs from the slow path (or behavior > > > before this patch). I'm tempted to add an expensive check to calculate > > > both the slow and fast path and fail when they differ, though the subtle > > > test changes here show there are slight differences already. > > > > > > So I guess we will need something like `HasAnyMaterializeTemporaryExpr` > > > from previous revisions of this patch. One thing I don't like about that > > > approach; IIRC if I accidentally omit methods of the Visitor, I think it > > > produces the wrong answers. Is there a better way to design such a > > > visitor? I'm assuming that if you don't define the method, then the > > > visitor stops descending further into the AST. Is that correct? > > Ideally, I think we fix LValueExprEvaluator::VisitMaterializeTemporaryExpr > > in ExprConstant so it only calls getOrCreateValue() and resets the value > > when we're actually evaluating the initializer of the corresponding > > variable. If we're not, we should bail out or treat it as a temporary. But > > when I looked briefly, I couldn't figure out how to check that condition > > correctly. If we do that, we don't need this workaround. > > > > It's possible to, instead of using a visitor pattern, use a switch > > statement that lists out all the possible kinds of expressions. Some places > > do that, I think, but I'm not sure how much it helps in this context; even > > if all the possible expressions are listed out, that doesn't mean you > > managed to classify them correctly. > > > > > though the subtle test changes here show there are slight differences > > > already > > > > That's probably fixable? The tests only show two kinds of difference; > > there can't be that many more. Not sure how much work it is. > > Ideally, I think we fix LValueExprEvaluator::VisitMaterializeTemporaryExpr > > in ExprConstant so it only calls getOrCreateValue() and resets the value > > when we're actually evaluating the initializer of the corresponding > > variable. If we're not, we should bail out or treat it as a temporary. But > > when I looked briefly, I couldn't figure out how to check that condition > > correctly. If we do that, we don't need this workaround. > > That sounds way less brittle than the `HasAnyMaterializeTemporaryExpr` I've > mocked up here, or any switch-based approach which would have similar > deficits IMO. > > Indeed, LValueExprEvaluator::VisitMaterializeTemporaryExpr gets called twice > for some reason. If I bail (`return false;` before calling > `getOrCreateValue`) the first but not the second, we produce the expected > result. > > AFAICT, the first call is via `Sema::CheckCompleteVariableDeclaration` > (there's like 20 stack frames in between) from the parser and the second is > `CodeGen::CodeGenModule::EmitGlobalVarDefinition` (~10 frames inbetween). > > I don't see how `LValueExprEvaluator::VisitMaterializeTemporaryExpr` could > have the context of its call chain like that, especially since there is so > many frames inbetween. Perhaps we can avoid calling > `LValueExprEvaluator::VisitMaterializeTemporaryExpr` that initial time > somehow? I suspect bailing out during Sema would have bad consequences. We need to be able to handle something like the following, which I think requires the evaluation from Sema to succeed: ``` typedef int x[2]; struct Z { int &&x, y; }; constexpr Z z = { x{1,2}[0], z.x=10 }; constexpr int *xx = &z.x; int *xxx = xx; ``` So we need to bail out for the second evaluation, not the first. I think we should be able to store the necessary context information in the EvalInfo, if it's not already there. 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
