CodeGen: Emit constant array temporaries into read-only globals.

[bkramer]

Summary

Instead of creating a copy on the stack just stash them in a private
constant global. This saves both the copying overhead and the stack
space, and gives the optimizer more room to constant fold.

This tries to make array temporaries more similar to regular arrays,
they can't use the same logic because a temporary has no VarDecl to be
bound to so we roll our own version here.

The original use case for this optimization was code like

for (int i : {1, 2, 3, 4, 5, 6, 7, 8, 10})
  foo(i);

where without this patch (assuming that the loop is not unrolled) we
would alloca an array on the stack, copy the 10 values over and
iterate on that. With this patch we put the array in .text use it
directly. Apart from that case this helps on virtually any passing of
a constant std::initializer_list as a function argument.

[majnemer]

It would be nice to give it internal linkage instead of private. That way the linker could zap these globals for targets which use COFF when they are unreferenced.

[• rafael]

This looks like a really nice improvement!

lib/CodeGen/CGExpr.cpp

313 ↗

(On Diff #21105)

If this function is in a COMDAT, this GV should be in it too.

[rsmith]

I support the direction here, but this patch is non-conforming. Please disable this under -fno-merge-all-constants, as we do for the similar case with a const local variable. (Generally, I think this should match what we do for const local variables to the extent possible.)

lib/CodeGen/CGExpr.cpp

308 ↗

(On Diff #21105)

For const locals, we also do this "optimization" for objects of record type (see CodeGenFunction::EmitAutoVarAlloca); any reason not to do the same here?

312 ↗

(On Diff #21105)

I don't think this assert is correct. For any expression where isConstantInitializer returns true, we should be able to emit the expression as a constant (well, even here there are known bugs which we work around by checking the type is POD), but I don't think the reverse is generally true. If CodeGen finds some unexpected way to emit an expression as a constant, that's OK.

315 ↗

(On Diff #21105)

This name doesn't make sense. All you know is that it's an array temporary.

[bkramer]

• Disable optimization if -fno-merge-constants is specified
• Enable optimization for records too and update test cases
• Remove bad assert
• Rename globals from ".initializer_list" to ".reftmp"
• Put new globals in the same COMDAT as the containing function

[rsmith]

Don't you also need a change to avoid performing the dynamic initialization of the temporary? (If not, where and how do we catch that case?)

[bkramer]

In D8034#133875, @rsmith wrote:

Don't you also need a change to avoid performing the dynamic initialization of the temporary? (If not, where and how do we catch that case?)

Not sure if I understand. If the temporary needs dynamic initialization then we can't constant fold the initializer and fall through to the existing CreateMemTemp call.

[rsmith]

In D8034#133876, @bkramer wrote:

Not sure if I understand. If the temporary needs dynamic initialization then we can't constant fold the initializer and fall through to the existing CreateMemTemp call.

Sure, but CreateMemTemp doesn't perform the initialization; that's done by the caller. How does the caller know to not do that?

[rsmith]

lib/CodeGen/CGExpr.cpp

390–395 ↗

(On Diff #21140)

Ah, here's where we deal with this. This code wasn't intended to handle the new case, but it it does the right thing, so ... OK then :)

Please update this comment to say "if the temporary is a global or is a constant temporary that we promoted to a global" or similar.

[bkramer]

Update comment

[rsmith]

I'm fine with this once you guys are in agreement on the linkage/comdat stuff.

test/CodeGenCXX/cxx0x-initializer-array.cpp

47 ↗

(On Diff #21147)

I'm not sure this test is still testing what it's supposed to test (that we emit dynamic zero initialization properly for array temporaries). Something like:

void g(p ptr) {
  f(a{ptr});
}

with the existing check for a store i32 -1, might work.

test/CodeGenCXX/cxx0x-initializer-references.cpp

39–54 ↗

(On Diff #21147)

Likewise here, changing one of the constants to a reference to a function parameter would better preserve the intention of the test.

test/CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp

67–68 ↗

(On Diff #21147)

Can you avoid testing the names of these globals?

79 ↗

(On Diff #21147)

Please don't check for the name of this type; that makes the test more fragile than necessary.

223–254 ↗

(On Diff #21147)

Please make both of these cases use a non-constant initializer; part of the point of these tests seems to be to check that we get the lifetime right.

[• rafael]

This is fine by me, just check with David about the linkage. It is not clear to me if using private on COFF right now would be just a missed optimization (and can be fixed in another patch) or a miscompile.

[bkramer]

Update test cases for Richard's feedback.

cfe/trunk/lib/CodeGen/CGExpr.cpp

 }
 
 static llvm::Value *
 createReferenceTemporary(CodeGenFunction &CGF,
                          const MaterializeTemporaryExpr *M, const Expr *Inner) {
   switch (M->getStorageDuration()) {
   case SD_FullExpression:
   case SD_Automatic:
+    // If we have a constant temporary array or record try to promote it into a
+    // constant global under the same rules a normal constant would've been
+    // promoted. This is easier on the optimizer and generally emits fewer
+    // instructions.
+    if (CGF.CGM.getCodeGenOpts().MergeAllConstants &&
+        (M->getType()->isArrayType() || M->getType()->isRecordType()) &&
+        CGF.CGM.isTypeConstant(M->getType(), true))
+      if (llvm::Constant *Init =
+              CGF.CGM.EmitConstantExpr(Inner, M->getType(), &CGF)) {
+        auto *GV = new llvm::GlobalVariable(
+            CGF.CGM.getModule(), Init->getType(), /*isConstant=*/true,
+            llvm::GlobalValue::PrivateLinkage, Init, ".ref.tmp");
+        GV->setAlignment(
+            CGF.getContext().getTypeAlignInChars(M->getType()).getQuantity());
+        // Put the new global in the same COMDAT the function containing the
+        // temporary is in.
+        GV->setComdat(CGF.CurFn->getComdat());
+        return GV;
+      }
     return CGF.CreateMemTemp(Inner->getType(), "ref.tmp");
 
   case SD_Thread:
   case SD_Static:
     return CGF.CGM.GetAddrOfGlobalTemporary(M, Inner);
 
   case SD_Dynamic:
     llvm_unreachable("temporary can't have dynamic storage duration");
if (opaque->getType()->isRecordType()) {
       assert(Adjustments.empty());
       return EmitOpaqueValueLValue(opaque);
     }
   }
 
   // Create and initialize the reference temporary.
   llvm::Value *Object = createReferenceTemporary(*this, M, E);
   if (auto *Var = dyn_cast<llvm::GlobalVariable>(Object)) {
-    // If the temporary is a global and has a constant initializer, we may
-    // have already initialized it.
+    // If the temporary is a global and has a constant initializer or is a
+    // constant temporary that we promoted to a global, we may have already
+    // initialized it.
     if (!Var->hasInitializer()) {
       Var->setInitializer(CGM.EmitNullConstant(E->getType()));
       EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true);
     }
   } else {
     EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true);
   }
   pushTemporaryCleanup(*this, M, E, Object);

cfe/trunk/test/CodeGenCXX/compound-literals.cpp

int f() {
   // CHECK-NEXT: [[RESULT:%[a-z0-9]+]] = load i32, i32*
   // CHECK-NEXT: call %struct.Y* @_ZN1YD1Ev
   // CHECK-NEXT: ret i32 [[RESULT]]
   return ((Y){17, "seventeen"}).i;
 }
 
 // CHECK-LABEL: define i32 @_Z1gv()
 int g() {
-  // CHECK: store [2 x i32]* %{{[a-z0-9.]+}}, [2 x i32]** [[V:%[a-z0-9.]+]]
+  // CHECK: store [2 x i32]* @{{.*}}, [2 x i32]** [[V:%[a-z0-9.]+]]
   const int (&v)[2] = (int [2]) {1,2};
 
   // CHECK: [[A:%[a-z0-9.]+]] = load [2 x i32]*, [2 x i32]** [[V]]
   // CHECK-NEXT: [[A0ADDR:%[a-z0-9.]+]] = getelementptr inbounds [2 x i32], [2 x i32]* [[A]], i32 0, {{.*}} 0
   // CHECK-NEXT: [[A0:%[a-z0-9.]+]] = load i32, i32* [[A0ADDR]]
   // CHECK-NEXT: ret i32 [[A0]]
   return v[0];
 }

cfe/trunk/test/CodeGenCXX/cxx0x-initializer-array.cpp

namespace ValueInitArrayOfMemPtr {
   void f(int n) {
     Agg1 a = { n };
     // CHECK: store i32 -1,
 
     Agg2 b = { n };
     // CHECK: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %{{.*}}, i8* bitcast ([3 x i32]* @[[THREE_NULL_MEMPTRS]] to i8*), i32 12, i32 4, i1 false)
   }
 
-  // CHECK-LABEL: define void @_ZN22ValueInitArrayOfMemPtr1gEv
-  void g() {
+  // Test dynamic initialization.
+  // CHECK-LABEL: define void @_ZN22ValueInitArrayOfMemPtr1gEMNS_1SEi
+  void g(p ptr) {
     // CHECK: store i32 -1,
-    f(a{});
+    f(a{ptr});
   }
 }
 
 namespace array_dtor {
   struct S { S(); ~S(); };
   using T = S[3];
   void f(const T &);
   void f(T *);

cfe/trunk/test/CodeGenCXX/cxx0x-initializer-references.cpp

void single_init() {
 
     using T = A&;
     // CHECK-NEXT: store %{{.*}}* %{{.*}}, %{{.*}}** %
     A &ra1b = T{a};
 
     // CHECK-NEXT: ret
   }
 
-  void reference_to_aggregate() {
+  void reference_to_aggregate(int i) {
     // CHECK: getelementptr {{.*}}, i32 0, i32 0
     // CHECK-NEXT: store i32 1
     // CHECK-NEXT: getelementptr {{.*}}, i32 0, i32 1
-    // CHECK-NEXT: store i32 2
+    // CHECK-NEXT: %[[I1:.*]] = load i32, i32*
+    // CHECK-NEXT: store i32 %[[I1]]
     // CHECK-NEXT: store %{{.*}}* %{{.*}}, %{{.*}}** %{{.*}}, align
-    const A &ra1{1, 2};
+    const A &ra1{1, i};
 
     // CHECK-NEXT: getelementptr inbounds [3 x i32], [3 x i32]* %{{.*}}, i{{32|64}} 0, i{{32|64}} 0
     // CHECK-NEXT: store i32 1
     // CHECK-NEXT: getelementptr inbounds i32, i32* %{{.*}}, i{{32|64}} 1
     // CHECK-NEXT: store i32 2
     // CHECK-NEXT: getelementptr inbounds i32, i32* %{{.*}}, i{{32|64}} 1
-    // CHECK-NEXT: store i32 3
+    // CHECK-NEXT: %[[I2:.*]] = load i32, i32*
+    // CHECK-NEXT: store i32 %[[I2]]
     // CHECK-NEXT: store [3 x i32]* %{{.*}}, [3 x i32]** %{{.*}}, align
-    const int (&arrayRef)[] = {1, 2, 3};
+    const int (&arrayRef)[] = {1, 2, i};
+
+    // CHECK: store %{{.*}}* @{{.*}}, %{{.*}}** %{{.*}}, align
+    const A &constra1{1, 2};
+
+    // CHECK-NEXT: store [3 x i32]* @{{.*}}, [3 x i32]** %{{.*}}, align
+    const int (&constarrayRef)[] = {1, 2, 3};
 
     // CHECK-NEXT: ret
   }
 
   struct B {
     B();
     ~B();
   };

cfe/trunk/test/CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp

 // CHECK: @_ZN15partly_constant1kE = global i32 0, align 4
 // CHECK: @_ZN15partly_constant2ilE = global {{.*}} null, align 8
 // CHECK: @[[PARTLY_CONSTANT_OUTER:_ZGRN15partly_constant2ilE.*]] = internal global {{.*}} zeroinitializer, align 8
 // CHECK: @[[PARTLY_CONSTANT_INNER:_ZGRN15partly_constant2ilE.*]] = internal global [3 x {{.*}}] zeroinitializer, align 8
 // CHECK: @[[PARTLY_CONSTANT_FIRST:_ZGRN15partly_constant2ilE.*]] = internal constant [3 x i32] [i32 1, i32 2, i32 3], align 4
 // CHECK: @[[PARTLY_CONSTANT_SECOND:_ZGRN15partly_constant2ilE.*]] = internal global [2 x i32] zeroinitializer, align 4
 // CHECK: @[[PARTLY_CONSTANT_THIRD:_ZGRN15partly_constant2ilE.*]] = internal constant [4 x i32] [i32 5, i32 6, i32 7, i32 8], align 4
 
+// CHECK: @[[REFTMP1:.*]] = private constant [2 x i32] [i32 42, i32 43], comdat($_ZN7PR204451fILi0EEEvv), align 4
+// CHECK: @[[REFTMP2:.*]] = private constant [3 x %{{.*}}] [%{{.*}} { i32 1 }, %{{.*}} { i32 2 }, %{{.*}} { i32 3 }], align 4
+
 // CHECK: appending global
 
 
 // thread_local initializer:
 // CHECK-LABEL: define internal void
 // CHECK: store i32* getelementptr inbounds ([4 x i32]* @_ZGRN25thread_local_global_array1xE_, i64 0, i64 0),
 // CHECK:       i32** getelementptr inbounds ({{.*}}* @_ZN25thread_local_global_array1xE, i32 0, i32 0), align 8
 // CHECK: store i64 4, i64* getelementptr inbounds ({{.*}}* @_ZN25thread_local_global_array1xE, i32 0, i32 1), align 8
void fn9() {
   // CHECK: call void @_ZN10destroyme1D1Ev
   // Only one destroy loop, since only one inner init list is directly inited.
   // CHECK-NOT: call void @_ZN10destroyme1D1Ev
   // CHECK: ret void
 }
 
 struct haslist1 {
   std::initializer_list<int> il;
-  haslist1();
+  haslist1(int i);
 };
 
-// CHECK-LABEL: define void @_ZN8haslist1C2Ev
-haslist1::haslist1()
+// CHECK-LABEL: define void @_ZN8haslist1C2Ei
+haslist1::haslist1(int i)
 // CHECK: alloca [3 x i32]
-// CHECK: store i32 1
+// CHECK: store i32 %
 // CHECK: store i32 2
 // CHECK: store i32 3
-// CHECK: store i{{32|64}} 3
-  : il{1, 2, 3}
+  : il{i, 2, 3}
 {
   destroyme2 dm2;
 }
 
 struct haslist2 {
   std::initializer_list<destroyme1> il;
   haslist2();
 };
 
 // CHECK-LABEL: define void @_ZN8haslist2C2Ev
 haslist2::haslist2()
   : il{destroyme1(), destroyme1()}
 {
   destroyme2 dm2;
   // CHECK: call void @_ZN10destroyme2D1Ev
   // CHECK: call void @_ZN10destroyme1D1Ev
 }
 
-void fn10() {
-  // CHECK-LABEL: define void @_Z4fn10v
+void fn10(int i) {
+  // CHECK-LABEL: define void @_Z4fn10i
   // CHECK: alloca [3 x i32]
   // CHECK: call noalias i8* @_Znw{{[jm]}}
-  // CHECK: store i32 1
+  // CHECK: store i32 %
   // CHECK: store i32 2
   // CHECK: store i32 3
   // CHECK: store i32*
-  // CHECK: store i{{32|64}} 3
-  (void) new std::initializer_list<int> {1, 2, 3};
+  (void) new std::initializer_list<int> {i, 2, 3};
 }
 
 void fn11() {
   // CHECK-LABEL: define void @_Z4fn11v
   (void) new std::initializer_list<destroyme1> {destroyme1(), destroyme1()};
   // CHECK: call void @_ZN10destroyme1D1Ev
   destroyme2 dm2;
   // CHECK: call void @_ZN10destroyme2D1Ev

 }
 
 namespace PR20445 {
   struct vector { vector(std::initializer_list<int>); };
   struct MyClass { explicit MyClass(const vector &v); };
   template<int x> void f() { new MyClass({42, 43}); }
   template void f<0>();
   // CHECK-LABEL: define {{.*}} @_ZN7PR204451fILi0EEEvv(
+  // CHECK: store i32* getelementptr inbounds ([2 x i32]* @[[REFTMP1]], i64 0, i64 0)
   // CHECK: call void @_ZN7PR204456vectorC1ESt16initializer_listIiE(
   // CHECK: call void @_ZN7PR204457MyClassC1ERKNS_6vectorE(
 }
+
+namespace ConstExpr {
+  class C {
+    int x;
+  public:
+    constexpr C(int x) : x(x) {}
+  };
+  void f(std::initializer_list<C>);
+  void g() {
+// CHECK-LABEL: _ZN9ConstExpr1gEv
+// CHECK: store %"class.ConstExpr::C"* getelementptr inbounds ([3 x %"class.ConstExpr::C"]* @[[REFTMP2]], i64 0, i64 0)
+// CHECK: call void @_ZN9ConstExpr1fESt16initializer_listINS_1CEE
+    f({C(1), C(2), C(3)});
+  }
+}