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

[nickdesaulniers]

Summary

As suggested by @efriedma in:
https://reviews.llvm.org/D76096#4370369

This should speed up evaluating whether an expression is constant or
not, but due to the complexity of these two different implementations,
we may start getting different answers for edge cases for which we do
not yet have test cases in-tree (or perhaps even performance regressions
for some cases). As such, contributors have carte blanche to revert if
necessary.

For additional historical context about ExprConstant vs CGExprConstant,
here's snippets from a private conversation on discord:

ndesaulniers:
why do we have clang/lib/AST/ExprConstant.cpp and
clang/lib/CodeGen/CGExprConstant.cpp? Does clang constant fold during
ast walking/creation AND during LLVM codegen?
efriedma:
originally, clang needed to handle two things: integer constant
expressions (the "5" in "int x[5];"), and constant global initializers
(the "5" in "int x = 5;").  pre-C++11, the two could be handled mostly
separately; so we had the code for integer constants in AST/, and the
code for globals in CodeGen/.  C++11 constexpr sort of destroyed that
separation, though. so now we do both kinds of constant evaluation on
the AST, then CGExprConstant translates the result of that evaluation
to LLVM IR.  but we kept around some bits of the old cgexprconstant to
avoid performance/memory usage regressions on large arrays.

[nickdesaulniers]

• also remove VisitExprWithCleanups, down to two outstanding test failures

[nickdesaulniers]

• one more test fix

[nickdesaulniers]

• fix string literals; still WIP

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

Maybe we should have a separate ConstExprEmitterLValue... trying to handle both LValues and RValues on the same codepath has been problematic in the past. It's very easy for code to get confused what it's actually trying to emit.

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1333

So we'd have a ConstExprEmitterLValue class with some visitor methods, and a ConstExprEmitterRValue with other methods implemented?

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

Something like that.

Actually thinking about it a bit more, not sure you need to actually implement ConstExprEmitterLValue for now. You might just be able to ensure we don't ever call ConstExprEmitter with an lvalue. The current ConstExprEmitter doesn't expect lvalues, and shouldn't call itself with lvalues. (We bail on explicit LValueToRValue conversions.) And Evaluate() shouldn't actually evaluate the contents of an lvalue if it isn't dereferenced, so there hopefully aren't any performance issues using that codepath.

In terms of implementation, I guess that's basically restoring the destType->isReferenceType() that got removed? (I know I suggested it, but I wasn't really thinking about it...)

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1333

One thing I think we may need to add to ConstExprEmitter is the ability to evaluate CallExprs based on certain test case failures...does that seem right?

1333

See also the calls to constexpr f() in clang/test/CodeGenCXX/const-init-cxx1y.cpp

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

The only things I know of that Evaluate() can't handle are CK_ToUnion, CK_ReinterpretMemberPointer, and DesignatedInitUpdateExpr. DesignatedInitUpdateExpr is related to the failures in test/CodeGenCXX/designated-init.cpp; I don't think the others show up in any of the testcases you've mentioned. (CK_ToUnion can't appear in C++ code. CK_ReinterpretMemberPointer is a reinterpret_cast<T> where T is a member pointer type.)

Given none of those constructs show up in const-init-cxx1y.cpp, the only reason for it to fail is if we aren't correctly falling back for a construct the current code doesn't know how to handle. You shouldn't need to implement any new constructs.

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1333

in clang/test/CodeGenCXX/designated-init.cpp we have:

>> 22 namespace ModifyStaticTemporary {                                               
   23   struct A { int &&temporary; int x; };                                         
   24   constexpr int f(int &r) { r *= 9; return r - 12; }                            
   25   A a = { 6, f(a.temporary) };

In the AST, that looks like:

| |-VarDecl 0x562b77df39b0 <line:25:3, col:29> col:5 used a 'A':'ModifyStaticTemporary::A' cinit
| | `-ExprWithCleanups 0x562b77df3c68 <col:9, col:29> 'A':'ModifyStaticTemporary::A'
| |   `-InitListExpr 0x562b77df3bb8 <col:9, col:29> 'A':'ModifyStaticTemporary::A'
| |     |-MaterializeTemporaryExpr 0x562b77df3c08 <col:11> 'int' xvalue extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
| |     | `-IntegerLiteral 0x562b77df3a18 <col:11> 'int' 6
| |     `-CallExpr 0x562b77df3b30 <col:14, col:27> 'int'
| |       |-ImplicitCastExpr 0x562b77df3b18 <col:14> 'int (*)(int &)' <FunctionToPointerDecay>
| |       | `-DeclRefExpr 0x562b77df3ad0 <col:14> 'int (int &)' lvalue Function 0x562b77df37a0 'f' 'int (int &)'
| |       `-MemberExpr 0x562b77df3aa0 <col:16, col:18> 'int' lvalue .temporary 0x562b77df35c0
| |         `-DeclRefExpr 0x562b77df3a80 <col:16> 'A':'ModifyStaticTemporary::A' lvalue Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'

(So, indeed no DesignatedInitUpdateExpr) but the call to the constexpr f() updates the reference (to 54). If I remove the visitor for MaterializeTemporaryExpr, we fail to evaluate f and end up emitting 6 rather than 54. Doesn't that mean that the fast path (ConstExprEmitter) needs to be able to evaluate CallExpr?

Or should VisitInitListExpr bail if any of the inits isa<MaterializeTemporaryExpr> (or perhaps isa<CallExpr>)?

[nickdesaulniers]

• just failing clang/test/CodeGenCXX/atomicinit.cpp

[nickdesaulniers]

• remove commented out code, still WIP

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

There are a few related cases here.

Case number one is when you have something like int z(); A a = { z(), z() };. There's no constant evaluation going on: you just emit two zero-initialized variables, and the runtime init initializes both of them.

Case number two is when everything is obviously constant: something like A a = { 1, 2 };

Case number three is when there are simple side-effects, and the standard requires we evaluate them at compile-time. Something like A a = { 1, ++a.temporary };. In this case, we need to ensure that we use Evaluate() to compute the value of both the temporary and the variable. The literal "1" is not the correct value to use. CodeGenModule::GetAddrOfGlobalTemporary is supposed to ensure we use the value from the evaluation of the variable as a whole (see comment "If the initializer of the extending declaration").

Case number four is when we can't constant-evaluate a variable as a whole, but we do evaluate some of the temporaries involved. Something like int z(); A a = { 1, a.temporary += z() }; In this case, we constant-evaluate the temporary using the initial value, then emit runtime initialization to finish computing the value of the variable as a whole.

You example should fall under case three. Both the temporary and the variable should be evaluated by Evaluate().

I'm not sure how the code ends up emitting the value 6, but hopefully that helps?

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1209

ConstExprEmitter should inherit an identical implementation of VisitImplicitCastExpr from StmtVisitor, I think.

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

Oh, I think I see what's happening; the code that looks for the temporary in GetAddrOfGlobalTemporary isn't reliable if the whole variable isn't evaluated first. It ends up pulling out the result of a partial evaluation, or something like that.

Making EmitArrayInitialization/EmitRecordInitialization bail if they see a MaterializeTemporaryExpr should deal with the issue, I think? Not sure if you'd need to recursively visit all the initializers (I don't remember what constructs allow lifetime extension off the top of my head).

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

To be more precise, what happens is that calling EvaluateAsLValue on the MaterializeTemporaryExpr actually *corrupts* the computed value of the temporary: the complete variable is evaluated earlier for other reasons, then EvaluateAsLValue overwrites the correct value we computed earlier with the wrong value.

[nickdesaulniers]

• remove unnecessary Visit*CastExpr overrides

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1333

In this case, we need to ensure that we use Evaluate() to compute the value of both the temporary and the variable.

Just triple checking, Evaluate is the "slow path" (i.e. VarDecl::evaluateValue, Expr::EvaluateAsLValue, and Expr::EvaluateAsRValue?

To be more precise, what happens is that calling EvaluateAsLValue on the MaterializeTemporaryExpr actually *corrupts* the computed value of the temporary

So the slow path gets it wrong? But prior to this patch, that's was used first before ConstExprEmitter? (Maybe I should add more comments about fast vs slow path)

---

Not sure if you'd need to recursively visit all the initializers (I don't remember what constructs allow lifetime extension off the top of my head).

I think I would; the last test case currently failing is clang/test/CodeGenCXX/atomicinit.cpp:

struct X {
  constexpr X(int n) : n(n) {}
  short n;
  char c = 6;
};

struct Y {
  _Atomic(X) a;
  _Atomic(int) b;
};
Y y = { X(4), 5 };

The AST for y looks like:

`-VarDecl 0x562bba0cad00 <line:11:1, col:17> col:3 y 'Y':'Y' cinit
  `-ExprWithCleanups 0x562bba0e9f28 <col:7, col:17> 'Y':'Y'
    `-InitListExpr 0x562bba0e9c20 <col:7, col:17> 'Y':'Y'
      |-ImplicitCastExpr 0x562bba0e9ef8 <col:9, col:12> '_Atomic(X)' <NonAtomicToAtomic>
      | `-ImplicitCastExpr 0x562bba0e9ee0 <col:9, col:12> 'X':'X' <ConstructorConversion>
      |   `-CXXConstructExpr 0x562bba0e9eb0 <col:9, col:12> 'X':'X' 'void (X &&) noexcept' elidable
      |     `-MaterializeTemporaryExpr 0x562bba0e9c70 <col:9, col:12> 'X':'X' xvalue
      |       `-CXXFunctionalCastExpr 0x562bba0e9b88 <col:9, col:12> 'X':'X' functional cast to X <ConstructorConversion>
      |         `-CXXConstructExpr 0x562bba0e9a10 <col:9, col:12> 'X':'X' 'void (int)'
      |           `-IntegerLiteral 0x562bba0cadc0 <col:11> 'int' 4
      `-ImplicitCastExpr 0x562bba0e9f10 <col:15> '_Atomic(int)' <NonAtomicToAtomic>
        `-IntegerLiteral 0x562bba0e9bb0 <col:15> 'int' 5

so we'd need to peek through the casts to find that there was a MaterializeTemporaryExpr in there, then bail?

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

Just triple checking, Evaluate is the "slow path" (i.e. VarDecl::evaluateValue, Expr::EvaluateAsLValue, and Expr::EvaluateAsRValue?

Yes.

To be more precise, what happens is that calling EvaluateAsLValue on the MaterializeTemporaryExpr actually *corrupts* the computed value of the temporary

So the slow path gets it wrong? But prior to this patch, that's was used first before ConstExprEmitter? (Maybe I should add more comments about fast vs slow path)

The EvaluateAsLValue bug only shows up if you EvaluateAsLValue pieces of the initializer. If you use the slow path first, we never EvaluateAsLValue pieces of the initializer; we just evaluate the whole variable initializer in one evaluation. (Depending on the construct, we may have to evaluate it during semantic analysis; if we do, the evaluation is cached.)

---

I think I would; the last test case currently failing is clang/test/CodeGenCXX/atomicinit.cpp:

I don't think that's the same issue? There, the MaterializeTemporaryExpr is immediately passed to a CXXConstructExpr, which returns an rvalue, so the final IR shouldn't actually reference the temporary. It looks like the issue is that VisitCXXConstructExpr is broken; it tries to look through a trivial move constructor, but the the operand of a move constructor is an lvalue, so the recursive visit doesn't work correctly. The following crashes even without your patch:

struct X {
  constexpr X(int n) : n(n) {}
  short n;
  char c = 6;
};
struct Y {
  _Atomic(X) a;
  int b;
};
int z;
Y y = { X(4), z };

You can probably just kill off the VisitCXXConstructExpr codepath... or if you want to try to repair it, I guess you can teach it to specifically handle only trivial constructors where the operand is a MaterializeTemporaryExpr.

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1333

You can probably just kill off the VisitCXXConstructExpr codepath..

If I delete ConstExprEmitter::VisitCXXConstructExpr (the "fast path") then wont the slow path then fail in the same way as your example above that crashes even without my patch?

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1333

I can't parse the question. What do you think is causing the _Atomic case to crash?

I tried looking a bit more; my example might not actually be related to the CXXConstructExpr. There's something weird going on with the NonAtomicToAtomic cast.

[nickdesaulniers]

• recursively check for MaterializeTemporaryExpr from InitListExprs; all tests pass (that doesn't imply this patch is good though!)

[efriedma]

Two points I'm not sure about in the current version:

• Handling MaterializeTemporaryExpr in ConstExprEmitter doesn't make sense; ConstExprEmitter is not supposed to visit lvalues. (And I'm not sure what the new check is supposed to do; E->isGLValue() is always true for a MaterializeTemporaryExpr.) Maybe try something like the following?

diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index b0dd598..51cf69e 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -1215,11 +1215,6 @@ public:
     return Visit(E->getSubExpr(), T);
   }

-  llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
-                                                QualType T) {
-    return Visit(E->getSubExpr(), T);
-  }
-
   llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
     auto *CAT = CGM.getContext().getAsConstantArrayType(ILE->getType());
     assert(CAT && "can't emit array init for non-constant-bound array");
@@ -1322,7 +1317,14 @@ public:
       assert(CGM.getContext().hasSameUnqualifiedType(Ty, Arg->getType()) &&
              "argument to copy ctor is of wrong type");

-      return Visit(Arg, Ty);
+      if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Arg)) {
+        // Look through the temporary; it's just converting the value to an
+        // lvalue to pass it to the constructor.
+        return Visit(MTE->getSubExpr(), Ty);
+      }
+
+      // Don't try to support arbitrary lvalue-to-rvalue conversions for now.
+      return nullptr;
     }

     return CGM.EmitNullConstant(Ty);

• HasAnyMaterializeTemporaryExpr shouldn't need to handle VisitCXXConstructExpr. The EvaluateAsLValue bug only applies to lifetime-extended temporaries. Any MaterializeTemporaryExpr that's the operand of a VisitCXXConstructExpr should not be lifetime-extended beyond the end of the expression. So it won't run into the EvaluateAsLValue issue. (https://en.cppreference.com/w/cpp/language/reference_initialization#Lifetime_of_a_temporary should be a complete list of cases where we might need to recurse; none of those corresponds to a CXXConstructExpr.)

[nickdesaulniers]

• use @efriedma's suggestion w/ dyn_cast

[nickdesaulniers]

In D151587#4397272, @efriedma wrote:

Two points I'm not sure about in the current version:

• Handling MaterializeTemporaryExpr in ConstExprEmitter doesn't make sense; ConstExprEmitter is not supposed to visit lvalues. (And I'm not sure what the new check is supposed to do; E->isGLValue() is always true for a MaterializeTemporaryExpr.) Maybe try something like the following?

Yeah, that works.

• HasAnyMaterializeTemporaryExpr shouldn't need to handle VisitCXXConstructExpr. The EvaluateAsLValue bug only applies to lifetime-extended temporaries. Any MaterializeTemporaryExpr that's the operand of a VisitCXXConstructExpr should not be lifetime-extended beyond the end of the expression. So it won't run into the EvaluateAsLValue issue. (https://en.cppreference.com/w/cpp/language/reference_initialization#Lifetime_of_a_temporary should be a complete list of cases where we might need to recurse; none of those corresponds to a CXXConstructExpr.)

See the AST in https://reviews.llvm.org/D151587#4396705; without a visitor for that, we don't recurse far enough down to see there is a MaterializeTemporaryExpr child in the sub-AST. Maybe that's because of my subclassing of ConstStmtVisitor returning bool, so if I don't implement a visitor, the default visit returns false (if I understand CRTP/Visitor-pattern correctly).

[efriedma]

The following also crashes, with no MaterializeTemporaryExpr involved.

struct X {
  short n;
  char c;
};

struct Y {
  _Atomic(X) a;
  _Atomic(int) b;
};
constexpr X x{};
int z;
Y y = { x, z };

[nickdesaulniers]

In D151587#4397446, @efriedma wrote:

The following also crashes, with no MaterializeTemporaryExpr involved.

struct X {
  short n;
  char c;
};

struct Y {
  _Atomic(X) a;
  _Atomic(int) b;
};
constexpr X x{};
int z;
Y y = { x, z };

Yeah, but not because of this patch; that's a pre-existing issue.

[efriedma]

Yeah, but not because of this patch; that's a pre-existing issue.

Right; the _Atomic crashes are a pre-existing issue unrelated to MaterializeTemporaryExpr, so you shouldn't be trying to solve it by messing with HasAnyMaterializeTemporaryExpr.

[ahatanak]

In D151587#4397499, @nickdesaulniers wrote:

In D151587#4397446, @efriedma wrote:

The following also crashes, with no MaterializeTemporaryExpr involved.

struct X {
  short n;
  char c;
};

struct Y {
  _Atomic(X) a;
  _Atomic(int) b;
};
constexpr X x{};
int z;
Y y = { x, z };

Yeah, but not because of this patch; that's a pre-existing issue.

Is there a github issue for this crash? Is anyone looking into this? The following code crashes too:

typedef union {
  unsigned int f0;
} Union0;

typedef struct {
  _Atomic(Union0) f1;
} Struct0;

Struct0 g = {};

It looks like there is a bug here: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L15066

Value is being discarded after the call to EvaluateAtomic.

[nickdesaulniers]

In D151587#4399684, @ahatanak wrote:

Is there a github issue for this crash?

I'm not aware, but I also haven't looked.

Is anyone looking into this?

I guess I am tangentially as it seems to be a blocker for this patch.

The following code crashes too:

typedef union {
  unsigned int f0;
} Union0;

typedef struct {
  _Atomic(Union0) f1;
} Struct0;

Struct0 g = {};

It looks like there is a bug here: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L15066

Value is being discarded after the call to EvaluateAtomic.

I have no idea if D152303 is correct, but PTAL. Thanks for identifying such code.

[nickdesaulniers]

• squash D151572 "up" into this, remove HasAnyMaterializeTemporaryExpr Visitor, check for MaterializeTemporaryExpr in InitListExpr instead

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1279

This needs a comment explaining why we're bailing out here.

1670–1671

Why are you changing this to "isLValueReferenceType"? I think rvalue references need to be handled basically the same way as lvalue references.

1670–1671

You can probably delete this FIXME comment.

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1279

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 };

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1279

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?

[nickdesaulniers]

• resurrect HasAnyMaterializeTemporaryExpr, add @efriedma's test case

[nickdesaulniers]

• remove fixme, restore isReferenceType

[nickdesaulniers]

• move added test case CHECK lines closer

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1279

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.

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

917

Should only need to visit base, not idx.

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1279

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?

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1279

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.

[nickdesaulniers]

clang/lib/CodeGen/CGExprConstant.cpp

1279

EvalInfo::EvalMode seems to differ between the first and second call to LValueExprEvaluator::VisitMaterializeTemporaryExpr. Perhaps that?

[nickdesaulniers]

• test for RValue and EvalMode in LValueExprEvaluator::VisitMaterializeTemporaryExpr rather than Visit MTE's in VisitImplicitValueInitExpr.
• reformat

[efriedma]

clang/lib/AST/ExprConstant.cpp

8389

Checking isLValue() doesn't make sense; consider:

struct R { mutable long x; };
struct Z { const R &x, y; };
Z z = { R{1}, z.x.x=10 };

Maybe also want to check for EM_IgnoreSideEffects? Not sure what cases, if any, that would affect.

We should probably check E->getStorageDuration() == SD_Static. The cases where it's a local temporary don't hit the getOrCreateValue() codepath, so the evaluated value should be handled correctly.

Checking EvalMode feels a little weird, but I guess it should do the right thing in the cases I can think of? I'd like a second opinion on this.

[arsenm]

clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

60

This should have stayed a cast from generic, 1->5 cast isn't defined

[efriedma]

clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

60

If it's not supposed to behave this way, it's a bug in LLVM constant folding; the following folds the same way if you pass it to opt -S. Given that, I don't think it should block this patch. (I think the relevant code is CastInst::isEliminableCastPair.)

target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8"
target triple = "amdgcn"
@fold_priv = local_unnamed_addr addrspace(1) global ptr addrspace(5) addrspacecast (ptr addrspacecast(ptr addrspace(1) null to ptr) to ptr addrspace(5)), align 4

[nickdesaulniers]

clang/lib/AST/ExprConstant.cpp

8389

Changing this condition to:

if (E->getStorageDuration() == SD_Static &&                                   
    Info.EvalMode == EvalInfo::EM_ConstantFold &&                             
    E->isXValue())                                                            
  return false;

allows all tests in tree to pass, but messes up the test case you posted above. I'm trying to sus out what else might be different about that test case...we should return false for that, but I'm not sure what's different about that case.

In particular, I was playing with E->isUsableInConstantExpressions and E->getLifetimeExtendedTemporaryDecl(), but getting your case to work, I end up regressing clang/test/SemaCXX/attr-require-constant-initialization.cpp...argh!!

[efriedma]

clang/lib/AST/ExprConstant.cpp

8389

Shouldn't that just be the following?

if (E->getStorageDuration() == SD_Static &&                                   
    Info.EvalMode == EvalInfo::EM_ConstantFold)                                                            
  return false;

A materialized temporary is always going to be either an LValue or an XValue, and the difference between the two isn't relevant here.

[nickdesaulniers]

clang/lib/AST/ExprConstant.cpp

8389

I wish it were that simple. Checking those two alone will produce failures in the following tests:

Failed Tests (2):

Clang :: CodeGenCXX/mangle-ms.cpp
Clang :: SemaCXX/attr-require-constant-initialization.cpp

error: 'error' diagnostics seen but not expected:

File /android0/llvm-project/clang/test/SemaCXX/attr-require-constant-initialization.cpp Line 92: variable does not have a constant initializer

as an example of one failure, which is basically:

void foo(void) {
  __attribute__((require_constant_initialization)) static const int &temp_init = 42;
}

specifically, -std=c++03 is the only language version that fails.

[nickdesaulniers]

clang/lib/AST/ExprConstant.cpp

8389

Oh, perhaps it should simply be:

if (Info.EvalMode == EvalInfo::EM_ConstantFold && E->isXValue())
  return false;

let me add your test case for that, too.

[nickdesaulniers]

• add @efriedma's test case
• check if the extending decl is not a reference, rather than for X or L values

[nickdesaulniers]

• reformat

[efriedma]

clang/lib/AST/ExprConstant.cpp

8389

It looks like that case is using Expr::isConstantInitializer, which uses EM_ConstantFold, which then blows up. No combination of the checks you're trying will let you distinguish between that case and the case you're trying to bail out; in both cases, the EvalMode is EvalMode, it's an lvalue, and the storage duration is static.

Maybe the code in question shouldn't be using isConstantInitializer at all.

Checking for a reference type doesn't solve anything; it just makes the issues more complicated to reproduce.

[nickdesaulniers]

clang/lib/AST/ExprConstant.cpp

8389

d572f82e490b alludes to Expr::isConstantInitializer being error prone...back in 2011...

Maybe the code in question shouldn't be using isConstantInitializer at all.

Which code? My patch doesn't introduce explicit calls to that method.

[efriedma]

clang/lib/AST/ExprConstant.cpp

8389

Which code?

Sema::CheckCompleteVariableDeclaration

[efriedma]

clang/lib/AST/ExprConstant.cpp

8389

Maybe we can add a special case for binding SD_Static MaterializeTemporaryExpr to references in Expr::isConstantInitializer, so it doesn't try to evaluate them. (Before the call to EvaluteAsLValue.)

[nickdesaulniers]

clang/lib/AST/ExprConstant.cpp

8389

Expr::isConstantInitializer is invoked for:

#define ATTR __attribute__((require_constant_initialization))
void x (void) {
  ATTR static const int &temp_init = 42;
}

it is not invoked for

struct R { mutable long x; };
struct Z { const R &x, y; };
Z z = { R{1}, z.x.x=10 };

so modifying Expr::isConstantInitializer rather than LValueExprEvaluator::VisitMaterializeTemporaryExpr will do nothing for the latter-case and produce incorrect results.

[efriedma]

This seems to work:

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 25d3535e..98b1e4d 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3319,6 +3319,10 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   // kill the second parameter.

   if (IsForRef) {
+    if (auto *EWC = dyn_cast<ExprWithCleanups>(this))
+      return EWC->getSubExpr()->isConstantInitializer(Ctx, true, Culprit);
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(this))
+      return MTE->getSubExpr()->isConstantInitializer(Ctx, false, Culprit);
     EvalResult Result;
     if (EvaluateAsLValue(Result, Ctx) && !Result.HasSideEffects)
       return true;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f0185d0..69b0d0d 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -8381,6 +8381,9 @@ bool LValueExprEvaluator::VisitCallExpr(const CallExpr *E) {

 bool LValueExprEvaluator::VisitMaterializeTemporaryExpr(
     const MaterializeTemporaryExpr *E) {
+  if (Info.EvalMode == EvalInfo::EM_ConstantFold)
+    return false;
+
   // Walk through the expression to find the materialized temporary itself.
   SmallVector<const Expr *, 2> CommaLHSs;
   SmallVector<SubobjectAdjustment, 2> Adjustments;
@@ -8388,8 +8391,8 @@ bool LValueExprEvaluator::VisitMaterializeTemporaryExpr(
       E->getSubExpr()->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);

   // If we passed any comma operators, evaluate their LHSs.
-  for (unsigned I = 0, N = CommaLHSs.size(); I != N; ++I)
-    if (!EvaluateIgnoredValue(Info, CommaLHSs[I]))
+  for (const Expr *E : CommaLHSs)
+    if (!EvaluateIgnoredValue(Info, E))
       return false;

   // A materialized temporary with static storage duration can appear within the
@@ -15472,7 +15475,7 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
   EStatus.Diag = &Notes;

   EvalInfo Info(Ctx, EStatus,
-                (IsConstantInitialization && Ctx.getLangOpts().CPlusPlus11)
+                (IsConstantInitialization && Ctx.getLangOpts().CPlusPlus)
                     ? EvalInfo::EM_ConstantExpression
                     : EvalInfo::EM_ConstantFold);
   Info.setEvaluatingDecl(VD, Value);

[nickdesaulniers]

In D151587#4516772, @efriedma wrote:

This seems to work:

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 25d3535e..98b1e4d 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3319,6 +3319,10 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   // kill the second parameter.

   if (IsForRef) {
+    if (auto *EWC = dyn_cast<ExprWithCleanups>(this))
+      return EWC->getSubExpr()->isConstantInitializer(Ctx, true, Culprit);
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(this))
+      return MTE->getSubExpr()->isConstantInitializer(Ctx, false, Culprit);

Indeed, but I wonder why only do this when IsForRef == true (narrator: it's not)? Also, why for MaterializeTemporaryExpr manually force the ForRef parameter to false?

---

It seems like perhaps we want to call EvaluateAsLValue in Expr::isConstantInitializer AFTER checking the statement class. Otherwise we're basically reimplementing that big switch statement again; at least two cases as suggested by your diff.

[efriedma]

Indeed, but I wonder why only do this when IsForRef == true (narrator: it's not)?

A MaterializeTemporaryExpr is an lvalue; it only makes sense in lvalue contexts.

Also, why for MaterializeTemporaryExpr manually force the ForRef parameter to false?

The operand to a MaterializeTemporaryExpr is an rvalue.

It seems like perhaps we want to call EvaluateAsLValue in Expr::isConstantInitializer AFTER checking the statement class. Otherwise we're basically reimplementing that big switch statement again; at least two cases as suggested by your diff.

I think we want to keep lvalue and rvalue handling separate, for the same reasons we want to keep it separate in other places. It's very easy to get the semantics wrong if you try to handle lvalues and rvalues in the same switch.

[nickdesaulniers]

In D151587#4519700, @efriedma wrote:

It seems like perhaps we want to call EvaluateAsLValue in Expr::isConstantInitializer AFTER checking the statement class. Otherwise we're basically reimplementing that big switch statement again; at least two cases as suggested by your diff.

I think we want to keep lvalue and rvalue handling separate, for the same reasons we want to keep it separate in other places. It's very easy to get the semantics wrong if you try to handle lvalues and rvalues in the same switch.

Switching the order of operations seems to work with a slight fix for the CastExprs. Let me post the diff and see what you think. Tests are green (FWIW). (I think that's cleaner than rechecking the Expr subclass and doing the same thing, but only for 2 cases).

[nickdesaulniers]

• reorder the check for IsForRef relative to the switch on statement class
• add @efriedma's suggestions

[nickdesaulniers]

• add more context to the commit description

[nickdesaulniers]

• refactor common tail from IsForRef check

[efriedma]

This seems to be close to a good state.

clang/lib/AST/Expr.cpp

3451

An CK_LValueToRValue conversion needs to change IsForRef (or else you're checking whether the address of the value is constant, not the value itself).

[nickdesaulniers]

clang/lib/AST/Expr.cpp

3469–3475

@efriedma should a few more of these cases be changed from passing false as the ForRef param of isConstantInitializer to IsForRef? If I change the rest of the cases except for MaterializeTemporaryExprClass then tests are still green.

[nickdesaulniers]

• force ForRef to false for CK_LValueToRValue CastExprs, as per @efriedma

[efriedma]

clang/lib/AST/Expr.cpp

3469–3475

One of the reasons I didn't go with this approach is that I really didn't want to think about this question...

We probably don't have good test coverage for the C++ constructs, since we don't really use isConstantInitializer() outside of C++03 mode.

[nickdesaulniers]

clang/lib/AST/Expr.cpp

3469–3475

It looks like I should be able to test this against all of Android, and against Google's internal C++ codebase. Let me work through that, and see if I can shake out additional tests cases to add here so that we have lower risk of regression.

[nickdesaulniers]

Cool, looks like the libc++ presubmit tests have already spotted some unintended changes. Will root cause+fix.

[nickdesaulniers]

clang/lib/AST/ExprConstant.cpp

8387–8388

removing these two lines fixes the following regressions:

Failed Tests (6):

llvm-libc++-shared.cfg.in :: std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp
llvm-libc++-shared.cfg.in :: std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp
llvm-libc++-shared.cfg.in :: std/strings/basic.string/string.cons/dtor.pass.cpp
llvm-libc++-shared.cfg.in :: std/strings/basic.string/string.cons/pointer_assignment.pass.cpp
llvm-libc++-shared.cfg.in :: std/strings/basic.string/string.modifiers/string_assign/pointer.pass.cpp
llvm-libc++-shared.cfg.in :: std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv.pass.cpp

The condition probably needs further specification. Let me see if I can work out what's missing.

[nickdesaulniers]

clang/lib/AST/ExprConstant.cpp

8387–8388

Might need to restore the E->getStorageDuration() == SD_Static && check.

[nickdesaulniers]

• restore E->getStorageDuration() == SD_Static check to fix libcxx regressions

[efriedma]

restore E->getStorageDuration() == SD_Static check to fix libcxx regressions

Makes sense... but it would be nice to add a testcase for whatever construct was triggering this.

clang/lib/AST/Expr.cpp

3451

Not sure what I was thinking when I wrote the original comment here. We need to bail on LValueToRValue here: the only way to correctly do an lvalue-to-rvalue conversion is to evaluate the operand as an lvalue, then convert the resulting lvalue to an rvalue. We need to use ExprConstant to do that.

3469–3475

I think I'd prefer to go back to the original way I wrote this. I really just don't want to think about the right way of translating the various new kinds of lvalues that will be hitting this codepath.

I experimented with actually removing the relevant call to isConstantInitializer (basically forcing C++03 code onto the C++11 codepath for global vars), but it seemed sort of invasive/risky. Maybe still worth considering at some point, I guess, but I don't want to mix too many things into this patch.

My point with the test coverage was that this primarily impacts -std=c++03, in a subtle way that won't be obvious in a lot of code. I'd be surprised if you have significant amounts of code at Google that's still compiled with -std=c++03.

[nickdesaulniers]

In D151587#4523571, @efriedma wrote:

restore E->getStorageDuration() == SD_Static check to fix libcxx regressions

Makes sense... but it would be nice to add a testcase for whatever construct was triggering this.

#include <string>
#include <tuple>
constexpr bool test() {
  std::tuple<int, char const*> a = {34, "hello world"};
  std::pair<long, std::string> p;
  p = a;
  return true;
}
int main(int, char**) {
  static_assert(test());
  return 0;
}

was the furthest I was able to reproduce. Not sure we should be using std:: in clang/test/Sema; wasn't really able to reduce further.

[nickdesaulniers]

clang/lib/AST/Expr.cpp

3451

when you say we need to bail on LValueToRValue here do you just return false for that case or what?

[efriedma]

clang/lib/AST/Expr.cpp

3451

Probably just "break;".

[nickdesaulniers]

clang/lib/AST/Expr.cpp

3451

I guess this thread is moot if I go back to the way this was previously written...(running tests on that now).

[nickdesaulniers]

• go back to Eli's sad (and boring) old way of doing things^U:wq

[efriedma]

LGTM

clang/lib/AST/Expr.cpp

bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   // ConstExprEmitter in CGExprConstant.cpp; if they don't match, it
   // will lead to unexpected results.  Like ConstExprEmitter, it falls back
   // to isEvaluatable most of the time.
   //
   // If we ever capture reference-binding directly in the AST, we can
   // kill the second parameter.
 
   if (IsForRef) {
+    if (auto *EWC = dyn_cast<ExprWithCleanups>(this))
+      return EWC->getSubExpr()->isConstantInitializer(Ctx, true, Culprit);
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(this))
+      return MTE->getSubExpr()->isConstantInitializer(Ctx, false, Culprit);
     EvalResult Result;
     if (EvaluateAsLValue(Result, Ctx) && !Result.HasSideEffects)
       return true;
     if (Culprit)
       *Culprit = this;
     return false;
   }
 
bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   case CXXDynamicCastExprClass:
   case CXXReinterpretCastExprClass:
   case CXXAddrspaceCastExprClass:
   case CXXConstCastExprClass: {
     const CastExpr *CE = cast<CastExpr>(this);
 
     // Handle misc casts we want to ignore.
     if (CE->getCastKind() == CK_NoOp ||
         CE->getCastKind() == CK_LValueToRValue ||
         CE->getCastKind() == CK_ToUnion ||
         CE->getCastKind() == CK_ConstructorConversion ||
         CE->getCastKind() == CK_NonAtomicToAtomic ||
         CE->getCastKind() == CK_AtomicToNonAtomic ||
         CE->getCastKind() == CK_NullToPointer ||
         CE->getCastKind() == CK_IntToOCLSampler)
       return CE->getSubExpr()->isConstantInitializer(Ctx, false, Culprit);
 
     break;
   }
   case MaterializeTemporaryExprClass:
     return cast<MaterializeTemporaryExpr>(this)
         ->getSubExpr()
         ->isConstantInitializer(Ctx, false, Culprit);
 
   case SubstNonTypeTemplateParmExprClass:
     return cast<SubstNonTypeTemplateParmExpr>(this)->getReplacement()
       ->isConstantInitializer(Ctx, false, Culprit);
   case CXXDefaultArgExprClass:
     return cast<CXXDefaultArgExpr>(this)->getExpr()
       ->isConstantInitializer(Ctx, false, Culprit);
   case CXXDefaultInitExprClass:
     return cast<CXXDefaultInitExpr>(this)->getExpr()
       ->isConstantInitializer(Ctx, false, Culprit);
   }
   // Allow certain forms of UB in constant initializers: signed integer
   // overflow and floating-point division by zero. We'll give a warning on
   // these, but they're common enough that we have to accept them.
   if (isEvaluatable(Ctx, SE_AllowUndefinedBehavior))
     return true;
   if (Culprit)
     *Culprit = this;

clang/lib/AST/ExprConstant.cpp

case Builtin::BImove_if_noexcept:
     break;
   }
 
   return ExprEvaluatorBaseTy::VisitCallExpr(E);
 }
 
 bool LValueExprEvaluator::VisitMaterializeTemporaryExpr(
     const MaterializeTemporaryExpr *E) {
   // Walk through the expression to find the materialized temporary itself.
   SmallVector<const Expr *, 2> CommaLHSs;
   SmallVector<SubobjectAdjustment, 2> Adjustments;
   const Expr *Inner =
       E->getSubExpr()->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
 
   // If we passed any comma operators, evaluate their LHSs.
-  for (unsigned I = 0, N = CommaLHSs.size(); I != N; ++I)
-    if (!EvaluateIgnoredValue(Info, CommaLHSs[I]))
+  for (const Expr *E : CommaLHSs)
+    if (!EvaluateIgnoredValue(Info, E))
       return false;
 
   // A materialized temporary with static storage duration can appear within the
   // result of a constant expression evaluation, so we need to preserve its
   // value for use outside this evaluation.
   APValue *Value;
   if (E->getStorageDuration() == SD_Static) {
+    if (Info.EvalMode == EvalInfo::EM_ConstantFold)
+      return false;
     // FIXME: What about SD_Thread?
     Value = E->getOrCreateValue(true);
     *Value = APValue();
     Result.set(E);
   } else {
     Value = &Info.CurrentCall->createTemporary(
         E, E->getType(),
         E->getStorageDuration() == SD_FullExpression ? ScopeKind::FullExpression
bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
   if (isPRValue() && (getType()->isArrayType() || getType()->isRecordType()) &&
       !Ctx.getLangOpts().CPlusPlus11)
     return false;
 
   Expr::EvalStatus EStatus;
   EStatus.Diag = &Notes;
 
   EvalInfo Info(Ctx, EStatus,
-                (IsConstantInitialization && Ctx.getLangOpts().CPlusPlus11)
+                (IsConstantInitialization && Ctx.getLangOpts().CPlusPlus)
                     ? EvalInfo::EM_ConstantExpression
                     : EvalInfo::EM_ConstantFold);
   Info.setEvaluatingDecl(VD, Value);
   Info.InConstantContext = IsConstantInitialization;
 
   if (Info.EnableNewConstInterp) {
     auto &InterpCtx = const_cast<ASTContext &>(Ctx).getInterpContext();
     if (!InterpCtx.evaluateAsInitializer(Info, VD, Value))

clang/lib/CodeGen/CGExprConstant.cpp

bool ConstStructBuilder::UpdateStruct(ConstantEmitter &Emitter,
   return ConstStructBuilder(Emitter, Const, Offset)
       .Build(Updater, /*AllowOverwrite*/ true);
 }
 
 //===----------------------------------------------------------------------===//
 //                             ConstExprEmitter
 //===----------------------------------------------------------------------===//
 
 static ConstantAddress
 tryEmitGlobalCompoundLiteral(ConstantEmitter &emitter,
                              const CompoundLiteralExpr *E) {
   CodeGenModule &CGM = emitter.CGM;
   CharUnits Align = CGM.getContext().getTypeAlignInChars(E->getType());
   if (llvm::GlobalVariable *Addr =
           CGM.getAddrOfConstantCompoundLiteralIfEmitted(E))
     return ConstantAddress(Addr, Addr->getValueType(), Align);
 
llvm::Constant *VisitCastExpr(CastExpr *E, QualType destType) {
     case CK_ZeroToOCLOpaqueType:
     case CK_MatrixCast:
       return nullptr;
     }
     llvm_unreachable("Invalid CastKind");
   }
 
   llvm::Constant *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE, QualType T) {
     // No need for a DefaultInitExprScope: we don't handle 'this' in a
     // constant expression.
     return Visit(DIE->getExpr(), T);
   }
 
   llvm::Constant *VisitExprWithCleanups(ExprWithCleanups *E, QualType T) {
     return Visit(E->getSubExpr(), T);
   }
 
-  llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
-                                                QualType T) {
-    return Visit(E->getSubExpr(), T);
-  }
-
   llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
     auto *CAT = CGM.getContext().getAsConstantArrayType(ILE->getType());
     assert(CAT && "can't emit array init for non-constant-bound array");
     unsigned NumInitElements = ILE->getNumInits();
     unsigned NumElements = CAT->getSize().getZExtValue();
 
     // Initialising an array requires us to automatically
     // initialise any elements that have not been initialised explicitly
public:
   }
 
   llvm::Constant *VisitInitListExpr(InitListExpr *ILE, QualType T) {
     if (ILE->isTransparent())
       return Visit(ILE->getInit(0), T);
 
     if (ILE->getType()->isArrayType())
       return EmitArrayInitialization(ILE, T);
 
     if (ILE->getType()->isRecordType())
       return EmitRecordInitialization(ILE, T);
 
     return nullptr;
   }
 
   llvm::Constant *VisitDesignatedInitUpdateExpr(DesignatedInitUpdateExpr *E,
                                                 QualType destType) {
if (E->getNumArgs()) {
       assert(E->getNumArgs() == 1 && "trivial ctor with > 1 argument");
       assert(E->getConstructor()->isCopyOrMoveConstructor() &&
              "trivial ctor has argument but isn't a copy/move ctor");
 
       Expr *Arg = E->getArg(0);
       assert(CGM.getContext().hasSameUnqualifiedType(Ty, Arg->getType()) &&
              "argument to copy ctor is of wrong type");
 
-      return Visit(Arg, Ty);
+      // Look through the temporary; it's just converting the value to an
+      // lvalue to pass it to the constructor.
+      if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Arg))
+        return Visit(MTE->getSubExpr(), Ty);
+      // Don't try to support arbitrary lvalue-to-rvalue conversions for now.
+      return nullptr;
     }
 
     return CGM.EmitNullConstant(Ty);
   }
 
   llvm::Constant *VisitStringLiteral(StringLiteral *E, QualType T) {
     // This is a string literal initializing an array in an initializer.
     return CGM.GetConstantArrayFromStringLiteral(E);
   }
 
   llvm::Constant *VisitObjCEncodeExpr(ObjCEncodeExpr *E, QualType T) {
     // This must be an @encode initializing an array in a static initializer.
     // Don't emit it as the address of the string, emit the string data itself
     // as an inline array.
     std::string Str;
     CGM.getContext().getObjCEncodingForType(E->getEncodedType(), Str);
if (Ty->isRecordType())
         const CXXConstructorDecl *CD = E->getConstructor();
         if (CD->isTrivial() && CD->isDefaultConstructor())
           return CGM.EmitNullConstant(D.getType());
       }
   }
   InConstantContext = D.hasConstantInitialization();
 
   QualType destType = D.getType();
+  const Expr *E = D.getInit();
+  assert(E && "No initializer to emit");
+
+  if (!destType->isReferenceType()) {
+    QualType nonMemoryDestType = getNonMemoryType(CGM, destType);
+    if (llvm::Constant *C = ConstExprEmitter(*this).Visit(const_cast<Expr *>(E),
+                                                          nonMemoryDestType))
+      return emitForMemory(C, destType);
+  }
 
   // Try to emit the initializer.  Note that this can allow some things that
   // are not allowed by tryEmitPrivateForMemory alone.
-  if (auto value = D.evaluateValue()) {
+  if (APValue *value = D.evaluateValue())
     return tryEmitPrivateForMemory(*value, destType);
-  }
 
-  // FIXME: Implement C++11 [basic.start.init]p2: if the initializer of a
-  // reference is a constant expression, and the reference binds to a temporary,
-  // then constant initialization is performed. ConstExprEmitter will
-  // incorrectly emit a prvalue constant in this case, and the calling code
-  // interprets that as the (pointer) value of the reference, rather than the
-  // desired value of the referee.
-  if (destType->isReferenceType())
-    return nullptr;
-
-  const Expr *E = D.getInit();
-  assert(E && "No initializer to emit");
-
-  auto nonMemoryDestType = getNonMemoryType(CGM, destType);
-  auto C =
-    ConstExprEmitter(*this).Visit(const_cast<Expr*>(E), nonMemoryDestType);
-  return (C ? emitForMemory(C, destType) : nullptr);
 }
 
 llvm::Constant *
 ConstantEmitter::tryEmitAbstractForMemory(const Expr *E, QualType destType) {
   auto nonMemoryDestType = getNonMemoryType(CGM, destType);
   auto C = tryEmitAbstract(E, nonMemoryDestType);
   return (C ? emitForMemory(C, destType) : nullptr);
 }
llvm::Constant *ConstantEmitter::emitForMemory(CodeGenModule &CGM,
 
   return C;
 }
 
 llvm::Constant *ConstantEmitter::tryEmitPrivate(const Expr *E,
                                                 QualType destType) {
   assert(!destType->isVoidType() && "can't emit a void constant");
 
+  if (llvm::Constant *C =
+          ConstExprEmitter(*this).Visit(const_cast<Expr *>(E), destType))
+    return C;
+
   Expr::EvalResult Result;
 
   bool Success = false;
 
   if (destType->isReferenceType())
     Success = E->EvaluateAsLValue(Result, CGM.getContext());
   else
     Success = E->EvaluateAsRValue(Result, CGM.getContext(), InConstantContext);
 
-  llvm::Constant *C;
   if (Success && !Result.HasSideEffects)
-    C = tryEmitPrivate(Result.Val, destType);
-  else
-    C = ConstExprEmitter(*this).Visit(const_cast<Expr*>(E), destType);
+    return tryEmitPrivate(Result.Val, destType);
 
-  return C;
+  return nullptr;
 }
 
 llvm::Constant *CodeGenModule::getNullPointer(llvm::PointerType *T, QualType QT) {
   return getTargetCodeGenInfo().getNullPointer(*this, T, QT);
 }
 
 namespace {
 /// A struct which can be used to peephole certain kinds of finalization

clang/test/CodeGenCXX/const-init-cxx11.cpp

namespace BaseClass {
   // CHECK: @_ZN9BaseClass3dndE ={{.*}} constant {{.*}} { double 4.000000e+00, i32 3, double 4.000000e+00 }
   extern constexpr DND dnd = DND();
   // Note, N subobject is laid out in DN subobject's tail padding.
   // CHECK: @_ZN9BaseClass3dnnE ={{.*}} constant {{.*}} { double 4.000000e+00, i32 3, i32 3 }
   extern constexpr DNN dnn = DNN();
 
   struct E {};
   struct Test2 : X<E,0>, X<E,1>, X<E,2>, X<E,3> {};
-  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} undef
+  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} zeroinitializer, align 1
   extern constexpr Test2 t2 = Test2();
 
   struct __attribute((packed)) PackedD { double y = 2; };
   struct Test3 : C, PackedD { constexpr Test3() {} };
   // CHECK: @_ZN9BaseClass2t3E ={{.*}} constant <{ i8, double }> <{ i8 1, double 2.000000e+00 }>
   extern constexpr Test3 t3 = Test3();
 }
 

clang/test/CodeGenCXX/const-init-cxx1y.cpp

namespace ModifyStaticTemporary {
   A b = { 7, ++b.temporary };
   // CHECK: @_ZGRN21ModifyStaticTemporary1bE_ = internal global i32 8
   // CHECK: @_ZN21ModifyStaticTemporary1bE ={{.*}} global {{.*}} ptr @_ZGRN21ModifyStaticTemporary1bE_, i32 8
 
   // Can't emit all of 'c' as a constant here, so emit the initial value of
   // 'c.temporary', not the value as modified by the partial evaluation within
   // the initialization of 'c.x'.
   A c = { 10, (++c.temporary, b.x) };
-  // CHECK: @_ZGRN21ModifyStaticTemporary1cE_ = internal global i32 10
   // CHECK: @_ZN21ModifyStaticTemporary1cE ={{.*}} global {{.*}} zeroinitializer
+  // CHECK: @_ZGRN21ModifyStaticTemporary1cE_ = internal global i32 10
 }
 
 // CHECK: @_ZGRN28VariableTemplateWithConstRef1iIvEE_ = linkonce_odr constant i32 5, align 4
 // CHECK: @_ZN28VariableTemplateWithConstRef3useE ={{.*}} constant ptr @_ZGRN28VariableTemplateWithConstRef1iIvEE_
 namespace VariableTemplateWithConstRef {
   template <typename T>
   const int &i = 5;
   const int &use = i<void>;
namespace VariableTemplateWithPack {
   struct S {
     A &&a, &&b, &&c, &&d;
   };
   template <int... N>
   S &&s = {A{N}...};
   S *p = &s<1, 2, 3, 4>;
 }
 
+
+// CHECK: @_ZGR1z_ ={{.*}} global [2 x i32] [i32 10, i32 2]
+// CHECK: @z = global { ptr, i32 } { ptr @_ZGR1z_, i32 10 }
+typedef int v[2];
+struct Z { int &&x, y; };
+Z z = { v{1,2}[0], z.x = 10 };
+
+// CHECK: @_ZGR2z2_ ={{.*}} global %struct.R { i64 10 }
+// @z = {{.}} global %struct.Z { ptr @_ZGR1z_, %struct.R { i64 10 } }
+struct R { mutable long x; };
+struct Z2 { const R &x, y; };
+Z2 z2 = { R{1}, z2.x.x = 10 };
+
 // CHECK: __cxa_atexit({{.*}} @_ZN1BD1Ev, {{.*}} @b
 
 // CHECK: define
 // CHECK-NOT: @_ZGRN21ModifyStaticTemporary1cE_
 // CHECK: store {{.*}} @_ZGRN21ModifyStaticTemporary1cE_, {{.*}} @_ZN21ModifyStaticTemporary1cE
 // CHECK: add
 // CHECK: store
 // CHECK: load {{.*}} @_ZN21ModifyStaticTemporary1bE
 // CHECK: store {{.*}} @_ZN21ModifyStaticTemporary1cE

clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

 generic char *generic_p_NULL = NULL;
 
 // Test constant folding of null pointer.
 // A null pointer should be folded to a null pointer in the target address space.
 
 // CHECK: @fold_generic ={{.*}} local_unnamed_addr addrspace(1) global ptr null, align 8
 generic int *fold_generic = (global int*)(generic float*)(private char*)0;
 
-// CHECK: @fold_priv ={{.*}} local_unnamed_addr addrspace(1) global ptr addrspace(5) addrspacecast (ptr null to ptr addrspace(5)), align 4
+// CHECK: @fold_priv ={{.*}} local_unnamed_addr addrspace(1) global ptr addrspace(5) addrspacecast (ptr addrspace(1) null to ptr addrspace(5)), align 4
 private short *fold_priv = (private short*)(generic int*)(global void*)0;
 
 // CHECK: @fold_priv_arith ={{.*}} local_unnamed_addr addrspace(1) global ptr addrspace(5) inttoptr (i32 9 to ptr addrspace(5)), align 4
 private char *fold_priv_arith = (private char*)0 + 10;
 
 // CHECK: @fold_local_arith ={{.*}} local_unnamed_addr addrspace(1) global ptr addrspace(3) inttoptr (i32 9 to ptr addrspace(3)), align 4
 local char *fold_local_arith = (local char*)0 + 10;