[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:
> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587