nickdesaulniers added inline comments.
================ Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa<MaterializeTemporaryExpr>(E)) + return nullptr; + ---------------- 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? 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
