nickdesaulniers added inline comments.
================ Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa<MaterializeTemporaryExpr>(E)) + return nullptr; + ---------------- efriedma wrote: > 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. EvalInfo::EvalMode seems to differ between the first and second call to LValueExprEvaluator::VisitMaterializeTemporaryExpr. Perhaps that? 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
