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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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.


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

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

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`.


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> 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.


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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.


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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).


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 528605.
nickdesaulniers added a comment.

- use @efriedma's suggestion w/ dyn_cast


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx11.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

Index: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
+++ clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -57,7 +57,7 @@
 // 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
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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
Index: clang/test/CodeGenCXX/const-init-cxx11.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx11.cpp
+++ clang/test/CodeGenCXX/const-init-cxx11.cpp
@@ -88,7 +88,7 @@
 
   struct E {};
   struct Test2 : X, X, X, X {};
-  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} undef
+  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} zeroinitializer, align 1
   extern constexpr Test2 t2 = Test2();
 
   struct __attribute((packed)) PackedD { double y = 2; };
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -910,6 +910,27 @@
   .Build(Updater, /*AllowOverwrite*/ true);
 }
 
+class HasAnyMaterializeTemporaryExpr :
+  public ConstStmtVisitor {
+public:
+  bool VisitImplicitCastExpr(const ImplicitCastExpr *I) {
+return Visit(I->getSubExpr());
+  }
+  bool VisitCXXConstructExpr(const CXXConstructExpr *C) {
+return llvm::any_of(C->arguments(), [this](const Expr *A){
+  return Visit(A);
+});
+  }
+  bool VisitInitListExpr(const InitListExpr *I) {
+return llvm::any_of(I->inits(), [this](const Expr *II) {
+  return Visit(II);
+});
+  }
+  bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *) {
+return true;
+  }
+};
+
 //===--===//
 // ConstExprEmitter
 //===--===//
@@ -1215,11 +1236,6 @@
 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");
@@ -1279,6 +1295,12 @@
 if (ILE->isTransparent())
   return Visit(ILE->getInit(0), T);
 
+// If the InitListExpr contains a MaterializeTemporaryExpr recursively,
+// bail...
+HasAnyMaterializeTemporaryExpr H;
+if (H.Visit(ILE))
+  return nullptr;
+
 if (ILE->getType()->isArrayType())
   return EmitArrayInitialization(ILE, T);
 
@@ -1322,7 +1344,12 @@
   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(Arg))
+return Visit(MTE->getSubExpr(), Ty);
+  // Don't try to support arbitrary lvalue-to-rvalue conversions for now.
+  

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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(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.)


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 528571.
nickdesaulniers added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx11.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

Index: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
+++ clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -57,7 +57,7 @@
 // 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
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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
Index: clang/test/CodeGenCXX/const-init-cxx11.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx11.cpp
+++ clang/test/CodeGenCXX/const-init-cxx11.cpp
@@ -88,7 +88,7 @@
 
   struct E {};
   struct Test2 : X, X, X, X {};
-  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} undef
+  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} zeroinitializer, align 1
   extern constexpr Test2 t2 = Test2();
 
   struct __attribute((packed)) PackedD { double y = 2; };
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -910,6 +910,27 @@
   .Build(Updater, /*AllowOverwrite*/ true);
 }
 
+class HasAnyMaterializeTemporaryExpr :
+  public ConstStmtVisitor {
+public:
+  bool VisitImplicitCastExpr(const ImplicitCastExpr *I) {
+return Visit(I->getSubExpr());
+  }
+  bool VisitCXXConstructExpr(const CXXConstructExpr *C) {
+return llvm::any_of(C->arguments(), [this](const Expr *A){
+  return Visit(A);
+});
+  }
+  bool VisitInitListExpr(const InitListExpr *I) {
+return llvm::any_of(I->inits(), [this](const Expr *II) {
+  return Visit(II);
+});
+  }
+  bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *) {
+return true;
+  }
+};
+
 //===--===//
 // ConstExprEmitter
 //===--===//
@@ -1216,8 +1237,10 @@
   }
 
   llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
-QualType T) {
-return Visit(E->getSubExpr(), T);
+  QualType T) {
+if (E->isGLValue())
+  return Visit(E->getSubExpr(), T);
+return nullptr;
   }
 
   llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
@@ -1279,6 +1302,12 @@
 if (ILE->isTransparent())
   return Visit(ILE->getInit(0), T);
 
+// If the InitListExpr contains a MaterializeTemporaryExpr recursively,
+// bail...
+HasAnyMaterializeTemporaryExpr H;
+if (H.Visit(ILE))
+  return nullptr;
+
 if (ILE->getType()->isArrayType())
   return EmitArrayInitialization(ILE, T);
 
@@ -1655,27 +1684,27 @@
 
   QualType destType = D.getType();
 
-  // Try to emit the initializer.  Note that this can allow some things that
-  // are not allowed by tryEmitPrivateForMemory alone.
-  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 

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > efriedma wrote:
> > > > > efriedma wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > efriedma wrote:
> > > > > > > > nickdesaulniers wrote:
> > > > > > > > > nickdesaulniers wrote:
> > > > > > > > > > efriedma wrote:
> > > > > > > > > > > nickdesaulniers wrote:
> > > > > > > > > > > > efriedma wrote:
> > > > > > > > > > > > > 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.
> > > > > > > > > > > > So we'd have a `ConstExprEmitterLValue` class with some 
> > > > > > > > > > > > visitor methods, and a `ConstExprEmitterRValue` with 
> > > > > > > > > > > > other methods implemented?
> > > > > > > > > > > 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...)
> > > > > > > > > > One thing I think we may need to add to `ConstExprEmitter` 
> > > > > > > > > > is the ability to evaluate `CallExpr`s based on certain 
> > > > > > > > > > test case failures...does that seem right?
> > > > > > > > > See also the calls to `constexpr f()` in 
> > > > > > > > > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > > > > > > > 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` 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.
> > > > > > > in clang/test/CodeGenCXX/designated-init.cpp we have:
> > > > > > > ```
> > > > > > > >> 22 namespace ModifyStaticTemporary {   
> > > > > > > >> 
> > > > > > >23   struct A { int & int x; }; 
> > > > > > > 
> > > > > > >24   constexpr int f(int ) { r *= 9; return r - 12; }
> > > > > > > 
> > > > > > >25   A a = { 6, f(a.temporary) };
> > > > > > > ```
> > > > > > > In the AST, that looks like:
> > > > > > > ```
> > > > > > > | |-VarDecl 0x562b77df39b0  col:5 used a 
> > > > > > > 'A':'ModifyStaticTemporary::A' cinit
> > > > > > > | | `-ExprWithCleanups 0x562b77df3c68  
> > > > > > > 'A':'ModifyStaticTemporary::A'
> > > > > > > | |   `-InitListExpr 0x562b77df3bb8  
> > > > > > > 'A':'ModifyStaticTemporary::A'
> > > > > > > | | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' 
> > > > > > > xvalue extended by Var 0x562b77df39b0 'a' 
> > > > > > > 'A':'ModifyStaticTemporary::A'
> > > > > > > | | | `-IntegerLiteral 0x562b77df3a18  'int' 6
> > > > > > > | | `-CallExpr 0x562b77df3b30  'int'
> > > > > > > | |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int 
> > > > > > > &)' 
> > > > > > > | |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' 
> > > > > > > lvalue Function 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

efriedma wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > efriedma wrote:
> > > > efriedma wrote:
> > > > > nickdesaulniers wrote:
> > > > > > efriedma wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > nickdesaulniers wrote:
> > > > > > > > > efriedma wrote:
> > > > > > > > > > nickdesaulniers wrote:
> > > > > > > > > > > efriedma wrote:
> > > > > > > > > > > > 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.
> > > > > > > > > > > So we'd have a `ConstExprEmitterLValue` class with some 
> > > > > > > > > > > visitor methods, and a `ConstExprEmitterRValue` with 
> > > > > > > > > > > other methods implemented?
> > > > > > > > > > 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...)
> > > > > > > > > One thing I think we may need to add to `ConstExprEmitter` is 
> > > > > > > > > the ability to evaluate `CallExpr`s based on certain test 
> > > > > > > > > case failures...does that seem right?
> > > > > > > > See also the calls to `constexpr f()` in 
> > > > > > > > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > > > > > > 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` 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.
> > > > > > in clang/test/CodeGenCXX/designated-init.cpp we have:
> > > > > > ```
> > > > > > >> 22 namespace ModifyStaticTemporary { 
> > > > > > >>   
> > > > > >23   struct A { int & int x; };   
> > > > > >   
> > > > > >24   constexpr int f(int ) { r *= 9; return r - 12; }  
> > > > > >   
> > > > > >25   A a = { 6, f(a.temporary) };
> > > > > > ```
> > > > > > In the AST, that looks like:
> > > > > > ```
> > > > > > | |-VarDecl 0x562b77df39b0  col:5 used a 
> > > > > > 'A':'ModifyStaticTemporary::A' cinit
> > > > > > | | `-ExprWithCleanups 0x562b77df3c68  
> > > > > > 'A':'ModifyStaticTemporary::A'
> > > > > > | |   `-InitListExpr 0x562b77df3bb8  
> > > > > > 'A':'ModifyStaticTemporary::A'
> > > > > > | | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' 
> > > > > > xvalue extended by Var 0x562b77df39b0 'a' 
> > > > > > 'A':'ModifyStaticTemporary::A'
> > > > > > | | | `-IntegerLiteral 0x562b77df3a18  'int' 6
> > > > > > | | `-CallExpr 0x562b77df3b30  'int'
> > > > > > | |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int 
> > > > > > &)' 
> > > > > > | |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' 
> > > > > > lvalue Function 0x562b77df37a0 'f' 'int (int &)'
> > > > > > | |   `-MemberExpr 0x562b77df3aa0  'int' lvalue 
> > > > > > .temporary 0x562b77df35c0
> > > > > > | | `-DeclRefExpr 

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

nickdesaulniers wrote:
> efriedma wrote:
> > efriedma wrote:
> > > efriedma wrote:
> > > > nickdesaulniers wrote:
> > > > > efriedma wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > efriedma wrote:
> > > > > > > > > nickdesaulniers wrote:
> > > > > > > > > > efriedma wrote:
> > > > > > > > > > > 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.
> > > > > > > > > > So we'd have a `ConstExprEmitterLValue` class with some 
> > > > > > > > > > visitor methods, and a `ConstExprEmitterRValue` with other 
> > > > > > > > > > methods implemented?
> > > > > > > > > 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...)
> > > > > > > > One thing I think we may need to add to `ConstExprEmitter` is 
> > > > > > > > the ability to evaluate `CallExpr`s based on certain test case 
> > > > > > > > failures...does that seem right?
> > > > > > > See also the calls to `constexpr f()` in 
> > > > > > > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > > > > > 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` 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.
> > > > > in clang/test/CodeGenCXX/designated-init.cpp we have:
> > > > > ```
> > > > > >> 22 namespace ModifyStaticTemporary {   
> > > > > >> 
> > > > >23   struct A { int & int x; }; 
> > > > > 
> > > > >24   constexpr int f(int ) { r *= 9; return r - 12; }
> > > > > 
> > > > >25   A a = { 6, f(a.temporary) };
> > > > > ```
> > > > > In the AST, that looks like:
> > > > > ```
> > > > > | |-VarDecl 0x562b77df39b0  col:5 used a 
> > > > > 'A':'ModifyStaticTemporary::A' cinit
> > > > > | | `-ExprWithCleanups 0x562b77df3c68  
> > > > > 'A':'ModifyStaticTemporary::A'
> > > > > | |   `-InitListExpr 0x562b77df3bb8  
> > > > > 'A':'ModifyStaticTemporary::A'
> > > > > | | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' 
> > > > > xvalue extended by Var 0x562b77df39b0 'a' 
> > > > > 'A':'ModifyStaticTemporary::A'
> > > > > | | | `-IntegerLiteral 0x562b77df3a18  'int' 6
> > > > > | | `-CallExpr 0x562b77df3b30  'int'
> > > > > | |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int &)' 
> > > > > 
> > > > > | |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' 
> > > > > lvalue Function 0x562b77df37a0 'f' 'int (int &)'
> > > > > | |   `-MemberExpr 0x562b77df3aa0  'int' lvalue 
> > > > > .temporary 0x562b77df35c0
> > > > > | | `-DeclRefExpr 0x562b77df3a80  
> > > > > 'A':'ModifyStaticTemporary::A' lvalue Var 0x562b77df39b0 'a' 
> > > > > 'A':'ModifyStaticTemporary::A'
> > > > > ```
> > > > > (So, indeed no `DesignatedInitUpdateExpr`) but the call to the 
> > > > > 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

efriedma wrote:
> efriedma wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > nickdesaulniers wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > efriedma wrote:
> > > > > > > > nickdesaulniers wrote:
> > > > > > > > > efriedma wrote:
> > > > > > > > > > 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.
> > > > > > > > > So we'd have a `ConstExprEmitterLValue` class with some 
> > > > > > > > > visitor methods, and a `ConstExprEmitterRValue` with other 
> > > > > > > > > methods implemented?
> > > > > > > > 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...)
> > > > > > > One thing I think we may need to add to `ConstExprEmitter` is the 
> > > > > > > ability to evaluate `CallExpr`s based on certain test case 
> > > > > > > failures...does that seem right?
> > > > > > See also the calls to `constexpr f()` in 
> > > > > > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > > > > 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` 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.
> > > > in clang/test/CodeGenCXX/designated-init.cpp we have:
> > > > ```
> > > > >> 22 namespace ModifyStaticTemporary { 
> > > > >>   
> > > >23   struct A { int & int x; };   
> > > >   
> > > >24   constexpr int f(int ) { r *= 9; return r - 12; }  
> > > >   
> > > >25   A a = { 6, f(a.temporary) };
> > > > ```
> > > > In the AST, that looks like:
> > > > ```
> > > > | |-VarDecl 0x562b77df39b0  col:5 used a 
> > > > 'A':'ModifyStaticTemporary::A' cinit
> > > > | | `-ExprWithCleanups 0x562b77df3c68  
> > > > 'A':'ModifyStaticTemporary::A'
> > > > | |   `-InitListExpr 0x562b77df3bb8  
> > > > 'A':'ModifyStaticTemporary::A'
> > > > | | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' xvalue 
> > > > extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
> > > > | | | `-IntegerLiteral 0x562b77df3a18  'int' 6
> > > > | | `-CallExpr 0x562b77df3b30  'int'
> > > > | |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int &)' 
> > > > 
> > > > | |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' lvalue 
> > > > Function 0x562b77df37a0 'f' 'int (int &)'
> > > > | |   `-MemberExpr 0x562b77df3aa0  'int' lvalue 
> > > > .temporary 0x562b77df35c0
> > > > | | `-DeclRefExpr 0x562b77df3a80  
> > > > '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 
> > > > 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 527978.
nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.

- remove unnecessary Visit*CastExpr overrides


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx11.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

Index: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
+++ clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -57,7 +57,7 @@
 // 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
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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
Index: clang/test/CodeGenCXX/const-init-cxx11.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx11.cpp
+++ clang/test/CodeGenCXX/const-init-cxx11.cpp
@@ -88,7 +88,7 @@
 
   struct E {};
   struct Test2 : X, X, X, X {};
-  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} undef
+  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} zeroinitializer, align 1
   extern constexpr Test2 t2 = Test2();
 
   struct __attribute((packed)) PackedD { double y = 2; };
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1216,8 +1216,10 @@
   }
 
   llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
-QualType T) {
-return Visit(E->getSubExpr(), T);
+  QualType T) {
+if (E->isLValue() || E->isXValue())
+  return Visit(E->getSubExpr(), T);
+return nullptr;
   }
 
   llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
@@ -1276,6 +1278,9 @@
   }
 
   llvm::Constant *VisitInitListExpr(InitListExpr *ILE, QualType T) {
+if (ILE->getNumInits() && isa(ILE->getInit(0)))
+return nullptr;
+
 if (ILE->isTransparent())
   return Visit(ILE->getInit(0), T);
 
@@ -1655,27 +1660,27 @@
 
   QualType destType = D.getType();
 
-  // Try to emit the initializer.  Note that this can allow some things that
-  // are not allowed by tryEmitPrivateForMemory alone.
-  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");
 
   QualType nonMemoryDestType = getNonMemoryType(CGM, destType);
-  if (llvm::Constant *C = ConstExprEmitter(*this).Visit(const_cast(E),
-nonMemoryDestType))
-return emitForMemory(C, destType);
+  if (!destType->isReferenceType())
+if (llvm::Constant *C = ConstExprEmitter(*this).Visit(const_cast(E),
+  nonMemoryDestType))
+  return emitForMemory(C, destType);
+
+  // Try to emit the initializer.  Note that this can allow some things that
+  // are not allowed by 

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

efriedma wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > nickdesaulniers wrote:
> > > > > nickdesaulniers wrote:
> > > > > > efriedma wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > efriedma wrote:
> > > > > > > > > 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.
> > > > > > > > So we'd have a `ConstExprEmitterLValue` class with some visitor 
> > > > > > > > methods, and a `ConstExprEmitterRValue` with other methods 
> > > > > > > > implemented?
> > > > > > > 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...)
> > > > > > One thing I think we may need to add to `ConstExprEmitter` is the 
> > > > > > ability to evaluate `CallExpr`s based on certain test case 
> > > > > > failures...does that seem right?
> > > > > See also the calls to `constexpr f()` in 
> > > > > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > > > 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` 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.
> > > in clang/test/CodeGenCXX/designated-init.cpp we have:
> > > ```
> > > >> 22 namespace ModifyStaticTemporary {   
> > > >> 
> > >23   struct A { int & int x; }; 
> > > 
> > >24   constexpr int f(int ) { r *= 9; return r - 12; }
> > > 
> > >25   A a = { 6, f(a.temporary) };
> > > ```
> > > In the AST, that looks like:
> > > ```
> > > | |-VarDecl 0x562b77df39b0  col:5 used a 
> > > 'A':'ModifyStaticTemporary::A' cinit
> > > | | `-ExprWithCleanups 0x562b77df3c68  
> > > 'A':'ModifyStaticTemporary::A'
> > > | |   `-InitListExpr 0x562b77df3bb8  
> > > 'A':'ModifyStaticTemporary::A'
> > > | | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' xvalue 
> > > extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
> > > | | | `-IntegerLiteral 0x562b77df3a18  'int' 6
> > > | | `-CallExpr 0x562b77df3b30  'int'
> > > | |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int &)' 
> > > 
> > > | |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' lvalue 
> > > Function 0x562b77df37a0 'f' 'int (int &)'
> > > | |   `-MemberExpr 0x562b77df3aa0  'int' lvalue 
> > > .temporary 0x562b77df35c0
> > > | | `-DeclRefExpr 0x562b77df3a80  
> > > '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` (or perhaps `isa`)?
> > There are a few related cases here.
> > 

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added reviewers: efriedma, rsmith.
efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

efriedma wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > efriedma wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > efriedma wrote:
> > > > > > > > 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.
> > > > > > > So we'd have a `ConstExprEmitterLValue` class with some visitor 
> > > > > > > methods, and a `ConstExprEmitterRValue` with other methods 
> > > > > > > implemented?
> > > > > > 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...)
> > > > > One thing I think we may need to add to `ConstExprEmitter` is the 
> > > > > ability to evaluate `CallExpr`s based on certain test case 
> > > > > failures...does that seem right?
> > > > See also the calls to `constexpr f()` in 
> > > > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > > 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` 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.
> > in clang/test/CodeGenCXX/designated-init.cpp we have:
> > ```
> > >> 22 namespace ModifyStaticTemporary { 
> > >>   
> >23   struct A { int & int x; };   
> >   
> >24   constexpr int f(int ) { r *= 9; return r - 12; }  
> >   
> >25   A a = { 6, f(a.temporary) };
> > ```
> > In the AST, that looks like:
> > ```
> > | |-VarDecl 0x562b77df39b0  col:5 used a 
> > 'A':'ModifyStaticTemporary::A' cinit
> > | | `-ExprWithCleanups 0x562b77df3c68  
> > 'A':'ModifyStaticTemporary::A'
> > | |   `-InitListExpr 0x562b77df3bb8  
> > 'A':'ModifyStaticTemporary::A'
> > | | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' xvalue 
> > extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
> > | | | `-IntegerLiteral 0x562b77df3a18  'int' 6
> > | | `-CallExpr 0x562b77df3b30  'int'
> > | |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int &)' 
> > 
> > | |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' lvalue 
> > Function 0x562b77df37a0 'f' 'int (int &)'
> > | |   `-MemberExpr 0x562b77df3aa0  'int' lvalue 
> > .temporary 0x562b77df35c0
> > | | `-DeclRefExpr 0x562b77df3a80  
> > '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` (or perhaps `isa`)?
> 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 
> 

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1209
+return VisitCastExpr(I, T);
+  }
+  llvm::Constant *VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr *C, 
QualType T) {

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


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > nickdesaulniers wrote:
> > > > > > efriedma wrote:
> > > > > > > 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.
> > > > > > So we'd have a `ConstExprEmitterLValue` class with some visitor 
> > > > > > methods, and a `ConstExprEmitterRValue` with other methods 
> > > > > > implemented?
> > > > > 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...)
> > > > One thing I think we may need to add to `ConstExprEmitter` is the 
> > > > ability to evaluate `CallExpr`s based on certain test case 
> > > > failures...does that seem right?
> > > See also the calls to `constexpr f()` in 
> > > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> > 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` 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.
> in clang/test/CodeGenCXX/designated-init.cpp we have:
> ```
> >> 22 namespace ModifyStaticTemporary {   
> >> 
>23   struct A { int & int x; }; 
> 
>24   constexpr int f(int ) { r *= 9; return r - 12; }
> 
>25   A a = { 6, f(a.temporary) };
> ```
> In the AST, that looks like:
> ```
> | |-VarDecl 0x562b77df39b0  col:5 used a 
> 'A':'ModifyStaticTemporary::A' cinit
> | | `-ExprWithCleanups 0x562b77df3c68  
> 'A':'ModifyStaticTemporary::A'
> | |   `-InitListExpr 0x562b77df3bb8  
> 'A':'ModifyStaticTemporary::A'
> | | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' xvalue 
> extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
> | | | `-IntegerLiteral 0x562b77df3a18  'int' 6
> | | `-CallExpr 0x562b77df3b30  'int'
> | |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int &)' 
> 
> | |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' lvalue 
> Function 0x562b77df37a0 'f' 'int (int &)'
> | |   `-MemberExpr 0x562b77df3aa0  'int' lvalue 
> .temporary 0x562b77df35c0
> | | `-DeclRefExpr 0x562b77df3a80  
> '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` (or perhaps `isa`)?
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 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 527925.
nickdesaulniers added a comment.

- remove commented out code, still WIP


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx11.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

Index: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
+++ clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -57,7 +57,7 @@
 // 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
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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
Index: clang/test/CodeGenCXX/const-init-cxx11.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx11.cpp
+++ clang/test/CodeGenCXX/const-init-cxx11.cpp
@@ -88,7 +88,7 @@
 
   struct E {};
   struct Test2 : X, X, X, X {};
-  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} undef
+  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} zeroinitializer, align 1
   extern constexpr Test2 t2 = Test2();
 
   struct __attribute((packed)) PackedD { double y = 2; };
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1204,6 +1204,15 @@
 }
 llvm_unreachable("Invalid CastKind");
   }
+  llvm::Constant *VisitImplicitCastExpr(ImplicitCastExpr *I, QualType T) {
+return VisitCastExpr(I, T);
+  }
+  llvm::Constant *VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr *C, QualType T) {
+return VisitCastExpr(C, T);
+  }
+  llvm::Constant *VisitCStyleCastExpr(CStyleCastExpr *C, QualType T) {
+return VisitCastExpr(C, T);
+  }
 
   llvm::Constant *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE, QualType T) {
 // No need for a DefaultInitExprScope: we don't handle 'this' in a
@@ -1216,8 +1225,10 @@
   }
 
   llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
-QualType T) {
-return Visit(E->getSubExpr(), T);
+  QualType T) {
+if (E->isLValue() || E->isXValue())
+  return Visit(E->getSubExpr(), T);
+return nullptr;
   }
 
   llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
@@ -1276,6 +1287,9 @@
   }
 
   llvm::Constant *VisitInitListExpr(InitListExpr *ILE, QualType T) {
+if (ILE->getNumInits() && isa(ILE->getInit(0)))
+return nullptr;
+
 if (ILE->isTransparent())
   return Visit(ILE->getInit(0), T);
 
@@ -1655,27 +1669,27 @@
 
   QualType destType = D.getType();
 
-  // Try to emit the initializer.  Note that this can allow some things that
-  // are not allowed by tryEmitPrivateForMemory alone.
-  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");
 
   QualType nonMemoryDestType = getNonMemoryType(CGM, destType);
-  

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 527924.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx11.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

Index: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
+++ clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -57,7 +57,7 @@
 // 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
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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
Index: clang/test/CodeGenCXX/const-init-cxx11.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx11.cpp
+++ clang/test/CodeGenCXX/const-init-cxx11.cpp
@@ -88,7 +88,7 @@
 
   struct E {};
   struct Test2 : X, X, X, X {};
-  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} undef
+  // CHECK: @_ZN9BaseClass2t2E ={{.*}} constant {{.*}} zeroinitializer, align 1
   extern constexpr Test2 t2 = Test2();
 
   struct __attribute((packed)) PackedD { double y = 2; };
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1204,6 +1204,19 @@
 }
 llvm_unreachable("Invalid CastKind");
   }
+  llvm::Constant *VisitImplicitCastExpr(ImplicitCastExpr *I, QualType T) {
+//if (I->isLValue())
+  //return nullptr;
+return VisitCastExpr(I, T);
+  }
+  llvm::Constant *VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr *C, QualType T) {
+//if (C->isLValue())
+  //return nullptr;
+return VisitCastExpr(C, T);
+  }
+  llvm::Constant *VisitCStyleCastExpr(CStyleCastExpr *C, QualType T) {
+return VisitCastExpr(C, T);
+  }
 
   llvm::Constant *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE, QualType T) {
 // No need for a DefaultInitExprScope: we don't handle 'this' in a
@@ -1216,8 +1229,10 @@
   }
 
   llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
-QualType T) {
-return Visit(E->getSubExpr(), T);
+  QualType T) {
+if (E->isLValue() || E->isXValue())
+  return Visit(E->getSubExpr(), T);
+return nullptr;
   }
 
   llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
@@ -1276,6 +1291,9 @@
   }
 
   llvm::Constant *VisitInitListExpr(InitListExpr *ILE, QualType T) {
+if (ILE->getNumInits() && isa(ILE->getInit(0)))
+return nullptr;
+
 if (ILE->isTransparent())
   return Visit(ILE->getInit(0), T);
 
@@ -1655,27 +1673,30 @@
 
   QualType destType = D.getType();
 
-  // Try to emit the initializer.  Note that this can allow some things that
-  // are not allowed by tryEmitPrivateForMemory alone.
-  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())

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

efriedma wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > nickdesaulniers wrote:
> > > > > efriedma wrote:
> > > > > > 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.
> > > > > So we'd have a `ConstExprEmitterLValue` class with some visitor 
> > > > > methods, and a `ConstExprEmitterRValue` with other methods 
> > > > > implemented?
> > > > 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...)
> > > One thing I think we may need to add to `ConstExprEmitter` is the ability 
> > > to evaluate `CallExpr`s based on certain test case failures...does that 
> > > seem right?
> > See also the calls to `constexpr f()` in 
> > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> 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` 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.
in clang/test/CodeGenCXX/designated-init.cpp we have:
```
>> 22 namespace ModifyStaticTemporary { 
>>   
   23   struct A { int & int x; };   
  
   24   constexpr int f(int ) { r *= 9; return r - 12; }  
  
   25   A a = { 6, f(a.temporary) };
```
In the AST, that looks like:
```
| |-VarDecl 0x562b77df39b0  col:5 used a 
'A':'ModifyStaticTemporary::A' cinit
| | `-ExprWithCleanups 0x562b77df3c68  
'A':'ModifyStaticTemporary::A'
| |   `-InitListExpr 0x562b77df3bb8  
'A':'ModifyStaticTemporary::A'
| | |-MaterializeTemporaryExpr 0x562b77df3c08  'int' xvalue 
extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
| | | `-IntegerLiteral 0x562b77df3a18  'int' 6
| | `-CallExpr 0x562b77df3b30  'int'
| |   |-ImplicitCastExpr 0x562b77df3b18  'int (*)(int &)' 

| |   | `-DeclRefExpr 0x562b77df3ad0  'int (int &)' lvalue Function 
0x562b77df37a0 'f' 'int (int &)'
| |   `-MemberExpr 0x562b77df3aa0  'int' lvalue .temporary 
0x562b77df35c0
| | `-DeclRefExpr 0x562b77df3a80  
'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` (or perhaps `isa`)?


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > 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.
> > > > So we'd have a `ConstExprEmitterLValue` class with some visitor 
> > > > methods, and a `ConstExprEmitterRValue` with other methods implemented?
> > > 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...)
> > One thing I think we may need to add to `ConstExprEmitter` is the ability 
> > to evaluate `CallExpr`s based on certain test case failures...does that 
> > seem right?
> See also the calls to `constexpr f()` in 
> clang/test/CodeGenCXX/const-init-cxx1y.cpp
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` 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.


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > 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.
> > > So we'd have a `ConstExprEmitterLValue` class with some visitor methods, 
> > > and a `ConstExprEmitterRValue` with other methods implemented?
> > 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...)
> One thing I think we may need to add to `ConstExprEmitter` is the ability to 
> evaluate `CallExpr`s based on certain test case failures...does that seem 
> right?
See also the calls to `constexpr f()` in 
clang/test/CodeGenCXX/const-init-cxx1y.cpp


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

efriedma wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > 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.
> > So we'd have a `ConstExprEmitterLValue` class with some visitor methods, 
> > and a `ConstExprEmitterRValue` with other methods implemented?
> 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...)
One thing I think we may need to add to `ConstExprEmitter` is the ability to 
evaluate `CallExpr`s based on certain test case failures...does that seem right?


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

nickdesaulniers wrote:
> efriedma wrote:
> > 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.
> So we'd have a `ConstExprEmitterLValue` class with some visitor methods, and 
> a `ConstExprEmitterRValue` with other methods implemented?
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...)


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

efriedma wrote:
> 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.
So we'd have a `ConstExprEmitterLValue` class with some visitor methods, and a 
`ConstExprEmitterRValue` with other methods implemented?


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
 // This is a string literal initializing an array in an initializer.
-return CGM.GetConstantArrayFromStringLiteral(E);
+return E->isLValue() ?
+  CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :

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.


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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 526210.
nickdesaulniers added a comment.

- fix string literals; still WIP


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenCXX/designated-init.cpp
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

Index: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
+++ clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -57,7 +57,7 @@
 // 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
Index: clang/test/CodeGenCXX/designated-init.cpp
===
--- clang/test/CodeGenCXX/designated-init.cpp
+++ clang/test/CodeGenCXX/designated-init.cpp
@@ -4,7 +4,8 @@
 struct A { int x, y[3]; };
 struct B { A a; };
 
-// CHECK: @b ={{.*}} global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+// CHECK: @b = global %struct.B zeroinitializer, align 4
+
 B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
 
 union U {
@@ -19,10 +20,10 @@
   C c;
 };
 
-// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+// CHECK: @d1 = global %struct.D zeroinitializer, align 4
 D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
 
-// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+// CHECK: @d2 = global %struct.D zeroinitializer, align 4
 D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
 
 struct Bitfield {
@@ -34,7 +35,7 @@
   int n;
   Bitfield b;
 };
-// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+// CHECK: @bitfield = global %struct.WithBitfield zeroinitializer, align 4
 WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
 
 struct String {
@@ -43,7 +44,7 @@
 struct WithString {
   String str;
 };
-// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+// CHECK: @string = global %struct.WithString zeroinitializer, align 1
 WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
 
 struct LargeArray {
@@ -52,7 +53,7 @@
 struct WithLargeArray {
   LargeArray arr;
 };
-// CHECK: @large ={{.*}} global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+// CHECK: @large = global %struct.WithLargeArray zeroinitializer, align 4
 WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
 
 union OverwritePaddingWithBitfield {
@@ -62,5 +63,6 @@
 struct WithOverwritePaddingWithBitfield {
   OverwritePaddingWithBitfield a;
 };
-// CHECK: @overwrite_padding ={{.*}} global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+// CHECK: @overwrite_padding = global %struct.WithOverwritePaddingWithBitfield zeroinitializer, align 1
+
 WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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: @_ZN21ModifyStaticTemporary1cE = global %"struct.ModifyStaticTemporary::A" zeroinitializer
   // CHECK: @_ZGRN21ModifyStaticTemporary1cE_ = internal global i32 10
-  // CHECK: @_ZN21ModifyStaticTemporary1cE ={{.*}} global {{.*}} zeroinitializer
 }
 
 // CHECK: @_ZGRN28VariableTemplateWithConstRef1iIvEE_ = linkonce_odr constant i32 5, align 4
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1211,15 +1211,6 @@
 return Visit(DIE->getExpr(), T);
   }
 
-  llvm::Constant *VisitExprWithCleanups(ExprWithCleanups *E, QualType T) {
-return Visit(E->getSubExpr(), T);
-  }
-
-  

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 526195.
nickdesaulniers added a comment.
Herald added subscribers: kerbowa, jvesely.

- one more test fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenCXX/designated-init.cpp
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl

Index: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
+++ clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -57,7 +57,7 @@
 // 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
Index: clang/test/CodeGenCXX/designated-init.cpp
===
--- clang/test/CodeGenCXX/designated-init.cpp
+++ clang/test/CodeGenCXX/designated-init.cpp
@@ -4,7 +4,8 @@
 struct A { int x, y[3]; };
 struct B { A a; };
 
-// CHECK: @b ={{.*}} global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+// CHECK: @b = global %struct.B zeroinitializer, align 4
+
 B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
 
 union U {
@@ -19,10 +20,10 @@
   C c;
 };
 
-// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+// CHECK: @d1 = global %struct.D zeroinitializer, align 4
 D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
 
-// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+// CHECK: @d2 = global %struct.D zeroinitializer, align 4
 D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
 
 struct Bitfield {
@@ -34,7 +35,7 @@
   int n;
   Bitfield b;
 };
-// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+// CHECK: @bitfield = global %struct.WithBitfield zeroinitializer, align 4
 WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
 
 struct String {
@@ -43,7 +44,7 @@
 struct WithString {
   String str;
 };
-// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+// CHECK: @string = global %struct.WithString zeroinitializer, align 1
 WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
 
 struct LargeArray {
@@ -52,7 +53,7 @@
 struct WithLargeArray {
   LargeArray arr;
 };
-// CHECK: @large ={{.*}} global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+// CHECK: @large = global %struct.WithLargeArray zeroinitializer, align 4
 WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
 
 union OverwritePaddingWithBitfield {
@@ -62,5 +63,6 @@
 struct WithOverwritePaddingWithBitfield {
   OverwritePaddingWithBitfield a;
 };
-// CHECK: @overwrite_padding ={{.*}} global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+// CHECK: @overwrite_padding = global %struct.WithOverwritePaddingWithBitfield zeroinitializer, align 1
+
 WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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: @_ZN21ModifyStaticTemporary1cE = global %"struct.ModifyStaticTemporary::A" zeroinitializer
   // CHECK: @_ZGRN21ModifyStaticTemporary1cE_ = internal global i32 10
-  // CHECK: @_ZN21ModifyStaticTemporary1cE ={{.*}} global {{.*}} zeroinitializer
 }
 
 // CHECK: @_ZGRN28VariableTemplateWithConstRef1iIvEE_ = linkonce_odr constant i32 5, align 4
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1211,15 +1211,6 @@
 return Visit(DIE->getExpr(), T);
   }
 
-  llvm::Constant *VisitExprWithCleanups(ExprWithCleanups *E, QualType T) {
-return 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 526192.
nickdesaulniers added a comment.

- also remove VisitExprWithCleanups, down to two outstanding test failures


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenCXX/designated-init.cpp

Index: clang/test/CodeGenCXX/designated-init.cpp
===
--- clang/test/CodeGenCXX/designated-init.cpp
+++ clang/test/CodeGenCXX/designated-init.cpp
@@ -4,7 +4,8 @@
 struct A { int x, y[3]; };
 struct B { A a; };
 
-// CHECK: @b ={{.*}} global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+// CHECK: @b = global %struct.B zeroinitializer, align 4
+
 B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
 
 union U {
@@ -19,10 +20,10 @@
   C c;
 };
 
-// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+// CHECK: @d1 = global %struct.D zeroinitializer, align 4
 D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
 
-// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+// CHECK: @d2 = global %struct.D zeroinitializer, align 4
 D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
 
 struct Bitfield {
@@ -34,7 +35,7 @@
   int n;
   Bitfield b;
 };
-// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+// CHECK: @bitfield = global %struct.WithBitfield zeroinitializer, align 4
 WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
 
 struct String {
@@ -43,7 +44,7 @@
 struct WithString {
   String str;
 };
-// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+// CHECK: @string = global %struct.WithString zeroinitializer, align 1
 WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
 
 struct LargeArray {
@@ -52,7 +53,7 @@
 struct WithLargeArray {
   LargeArray arr;
 };
-// CHECK: @large ={{.*}} global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+// CHECK: @large = global %struct.WithLargeArray zeroinitializer, align 4
 WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
 
 union OverwritePaddingWithBitfield {
@@ -62,5 +63,6 @@
 struct WithOverwritePaddingWithBitfield {
   OverwritePaddingWithBitfield a;
 };
-// CHECK: @overwrite_padding ={{.*}} global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+// CHECK: @overwrite_padding = global %struct.WithOverwritePaddingWithBitfield zeroinitializer, align 1
+
 WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -34,8 +34,8 @@
   // '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: @_ZN21ModifyStaticTemporary1cE = global %"struct.ModifyStaticTemporary::A" zeroinitializer
   // CHECK: @_ZGRN21ModifyStaticTemporary1cE_ = internal global i32 10
-  // CHECK: @_ZN21ModifyStaticTemporary1cE ={{.*}} global {{.*}} zeroinitializer
 }
 
 // CHECK: @_ZGRN28VariableTemplateWithConstRef1iIvEE_ = linkonce_odr constant i32 5, align 4
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1211,15 +1211,6 @@
 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");
@@ -1657,18 +1648,6 @@
 
   // Try to emit the initializer.  Note that this can allow some things that
   // are not allowed by tryEmitPrivateForMemory alone.
-  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 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
Herald added a project: All.
nickdesaulniers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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

Signed-off-by: Nick Desaulniers 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151587

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/const-init-cxx1y.cpp
  clang/test/CodeGenCXX/cxx0x-initializer-references.cpp
  clang/test/CodeGenCXX/designated-init.cpp

Index: clang/test/CodeGenCXX/designated-init.cpp
===
--- clang/test/CodeGenCXX/designated-init.cpp
+++ clang/test/CodeGenCXX/designated-init.cpp
@@ -4,7 +4,8 @@
 struct A { int x, y[3]; };
 struct B { A a; };
 
-// CHECK: @b ={{.*}} global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+// CHECK: @b = global %struct.B zeroinitializer, align 4
+
 B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
 
 union U {
@@ -19,10 +20,10 @@
   C c;
 };
 
-// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+// CHECK: @d1 = global %struct.D zeroinitializer, align 4
 D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
 
-// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+// CHECK: @d2 = global %struct.D zeroinitializer, align 4
 D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
 
 struct Bitfield {
@@ -34,7 +35,7 @@
   int n;
   Bitfield b;
 };
-// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+// CHECK: @bitfield = global %struct.WithBitfield zeroinitializer, align 4
 WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
 
 struct String {
@@ -43,7 +44,7 @@
 struct WithString {
   String str;
 };
-// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+// CHECK: @string = global %struct.WithString zeroinitializer, align 1
 WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
 
 struct LargeArray {
@@ -52,7 +53,7 @@
 struct WithLargeArray {
   LargeArray arr;
 };
-// CHECK: @large ={{.*}} global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+// CHECK: @large = global %struct.WithLargeArray zeroinitializer, align 4
 WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
 
 union OverwritePaddingWithBitfield {
@@ -62,5 +63,6 @@
 struct WithOverwritePaddingWithBitfield {
   OverwritePaddingWithBitfield a;
 };
-// CHECK: @overwrite_padding ={{.*}} global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+// CHECK: @overwrite_padding = global %struct.WithOverwritePaddingWithBitfield zeroinitializer, align 1
+
 WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: clang/test/CodeGenCXX/cxx0x-initializer-references.cpp
===
--- clang/test/CodeGenCXX/cxx0x-initializer-references.cpp
+++ clang/test/CodeGenCXX/cxx0x-initializer-references.cpp
@@ -6,7 +6,7 @@
 const char (_string_ref)[5] = {"hi"};
 
 // This binds directly to a string literal object.
-// CHECK: @nonextended_string_ref ={{.*}} constant ptr @.str
+// CHECK: @nonextended_string_ref = constant [3 x i8] c"hi\00", align 4
 const char (_string_ref)[3] = {"hi"};
 
 namespace reference {
Index: clang/test/CodeGenCXX/const-init-cxx1y.cpp
===
--- clang/test/CodeGenCXX/const-init-cxx1y.cpp
+++ clang/test/CodeGenCXX/const-init-cxx1y.cpp
@@ -23,11 +23,11 @@
   struct A { int & int x; };
   constexpr int f(int ) { r *= 9; return r - 12; }
   A a = { 6, f(a.temporary) };
-  // CHECK: @_ZGRN21ModifyStaticTemporary1aE_ = internal global i32 54
+  // CHECK: @_ZGRN21ModifyStaticTemporary1aE_ = internal global i32 6, align 4
   // CHECK: @_ZN21ModifyStaticTemporary1aE ={{.*}} global {{.*}} ptr @_ZGRN21ModifyStaticTemporary1aE_, i32 42
 
   A b = { 7, ++b.temporary };
-  // CHECK: @_ZGRN21ModifyStaticTemporary1bE_ = internal global i32 8
+  // CHECK: @_ZGRN21ModifyStaticTemporary1bE_ = internal global i32 7, align 4
   // CHECK: @_ZN21ModifyStaticTemporary1bE ={{.*}} global {{.*}} ptr @_ZGRN21ModifyStaticTemporary1bE_, i32 8
 
   // Can't emit all of 'c' as a constant here, so emit the initial value of
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1215,11 +1215,6 @@
 return Visit(E->getSubExpr(), T);
   }
 
-  llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
-