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

[Nick Desaulniers via Phabricator via cfe-commits]

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