[ObjC][CodeGen] Remove an assert that is no longer correct.

[ahatanak]

Summary

The assert was committed in r183967. After r231508 made changes to promote constant temporaries to globals, the assert fires when a std::initializer_list is constructed using Objective-C string literals:

void foo1() {

std::vector<NSString*> strs = {@"a", @"b"};

}

rdar://problem/25504992
rdar://problem/25955179

[manmanren]

After r231508 made changes to promote constant temporaries to globals, the assert fires when a std::initializer_list is constructed using Objective-C string literals.

--> Can you explain the code path after r231508 for your example? Will r231508 change the code path if we start with a global?
r231508 seems to change the behavior constant temporaries only.

Thanks for working on this!
Manman

lib/CodeGen/CGExpr.cpp

364 ↗

(On Diff #56492)

Can you add comments here?

[ahatanak]

Add a comment explaining why it is OK to return early if the global variable has an initializer.

[ahatanak]

In D20045#425903, @manmanren wrote:

After r231508 made changes to promote constant temporaries to globals, the assert fires when a std::initializer_list is constructed using Objective-C string literals.

--> Can you explain the code path after r231508 for your example? Will r231508 change the code path if we start with a global?
r231508 seems to change the behavior constant temporaries only.

I think your question is about the following case where initializer_list is used to initialize a global variable?

const std::vector<NSString*> CfgFiles = {@"Test0", @"Test1", @"Test2", @"Test3"};

This is not different from the case where the vector is declared inside a function. In both cases, a temporary std::initializer_list is created and passed to std::vector's constructor.
r231508 made changes to create a global array of strings (@.ref.tmp in the IR shown below).

@.ref.tmp = private constant [4 x %0*] [%0* bitcast (%struct.NSConstantString_tag* @_unnamed_cfstring_ to %0*), %0* bitcast (%struct.NSConstantString_tag* @_unnamed_cfstring_.2 to %0*), %0* bitcast (%struct.NSConstantString_tag* @_unnamed_cfstring_.4 to %0*), %0* bitcast (%struct.NSConstantString_tag* @_unnamed_cfstring_.6 to %0*)], align 8
...
%agg.tmp = alloca %"class.std::initializer_list", align 8 ; temporary std::initializer_list
%begin_ = getelementptr inbounds %"class.std::initializer_list", %"class.std::initializer_list"* %agg.tmp, i32 0, i32 0
store %0 getelementptr inbounds ([4 x %0*], [4 x %0*]* @.ref.tmp, i64 0, i64 0), %0* %begin_, align 8, !tbaa !7 ; std::initializer_list holds a pointer to the array.

[rjmccall]

lib/CodeGen/CGExpr.cpp

365 ↗

(On Diff #57088)

As we discussed in person, please make the comment address *why* this can ever have an initializer. I think something like this would be ok:

// createReferenceTemporary will promote the temporary to a global
// with a constant initializer if it can.  It can only do this to a value of
// ARC-manageable type if the value is global and therefore "immune"
// to ref-counting operations.  Therefore we have no need to emit either
// a dynamic initialization or a cleanup and we can just return the
// address of the temporary.

[ahatanak]

Thanks, I updated the comment in the new patch.

[rjmccall]

LGTM, thanks.

cfe/trunk/lib/CodeGen/CGExpr.cpp

EmitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *M) {
   if (ownership != Qualifiers::OCL_None &&
       ownership != Qualifiers::OCL_ExplicitNone) {
     Address Object = createReferenceTemporary(*this, M, E);
     if (auto *Var = dyn_cast<llvm::GlobalVariable>(Object.getPointer())) {
       Object = Address(llvm::ConstantExpr::getBitCast(Var,
                            ConvertTypeForMem(E->getType())
                              ->getPointerTo(Object.getAddressSpace())),
                        Object.getAlignment());
-      // We should not have emitted the initializer for this temporary as a
-      // constant.
-      assert(!Var->hasInitializer());
+
+      // createReferenceTemporary will promote the temporary to a global with a
+      // constant initializer if it can.  It can only do this to a value of
+      // ARC-manageable type if the value is global and therefore "immune" to
+      // ref-counting operations.  Therefore we have no need to emit either a
+      // dynamic initialization or a cleanup and we can just return the address
+      // of the temporary.
+      if (Var->hasInitializer())
+        return MakeAddrLValue(Object, M->getType(), AlignmentSource::Decl);
+
       Var->setInitializer(CGM.EmitNullConstant(E->getType()));
     }
     LValue RefTempDst = MakeAddrLValue(Object, M->getType(),
                                        AlignmentSource::Decl);
 
     switch (getEvaluationKind(E->getType())) {
     default: llvm_unreachable("expected scalar or aggregate expression");
     case TEK_Scalar:

cfe/trunk/test/CodeGenObjCXX/arc-cxx11-init-list.mm

 // RUN: %clang_cc1 -triple armv7-ios5.0 -std=c++11 -fobjc-arc -Os -emit-llvm -o - %s | FileCheck %s
 
+// CHECK: @[[STR0:.*]] = private unnamed_addr constant [5 x i8] c"str0\00", section "__TEXT,__cstring,cstring_literals"
+// CHECK: @[[UNNAMED_CFSTRING0:.*]] = private constant %struct.__NSConstantString_tag { i32* getelementptr inbounds ([0 x i32], [0 x i32]* @__CFConstantStringClassReference, i32 0, i32 0), i32 1992, i8* getelementptr inbounds ([5 x i8], [5 x i8]* @[[STR0]], i32 0, i32 0), i32 4 }, section "__DATA,__cfstring"
+// CHECK: @[[STR1:.*]] = private unnamed_addr constant [5 x i8] c"str1\00", section "__TEXT,__cstring,cstring_literals"
+// CHECK: @[[UNNAMED_CFSTRING1:.*]] = private constant %struct.__NSConstantString_tag { i32* getelementptr inbounds ([0 x i32], [0 x i32]* @__CFConstantStringClassReference, i32 0, i32 0), i32 1992, i8* getelementptr inbounds ([5 x i8], [5 x i8]* @[[STR1]], i32 0, i32 0), i32 4 }, section "__DATA,__cfstring"
+// CHECK: @[[REFTMP:.*]] = private constant [2 x i8*] [i8* bitcast (%struct.__NSConstantString_tag* @[[UNNAMED_CFSTRING0]] to i8*), i8* bitcast (%struct.__NSConstantString_tag* @[[UNNAMED_CFSTRING1]] to i8*)]
+
 typedef __SIZE_TYPE__ size_t;
 
 namespace std {
 template <typename _Ep>
 class initializer_list {
   const _Ep* __begin_;
   size_t __size_;
 

 
 extern "C" void multiple() { function({ [I new], [I new] }); }
 
 // CHECK: [[INSTANCE:%.*]] = {{.*}} call i8* bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to i8* (i8*, i8*)*)(i8* {{.*}}, i8* {{.*}})
 // CHECK-NEXT: [[CAST:%.*]] = bitcast [{{[0-9]+}} x %0*]* %{{.*}} to i8**
 // CHECK-NEXT: store i8* [[INSTANCE]], i8** [[CAST]],
 // CHECK: call void @objc_release(i8* {{.*}})
 
+std::initializer_list<id> foo1() {
+  return {@"str0", @"str1"};
+}
+
+// CHECK: define void @_Z4foo1v(%"class.std::initializer_list.0"* {{.*}} %[[AGG_RESULT:.*]])
+// CHECK: %[[BEGIN:.*]] = getelementptr inbounds %"class.std::initializer_list.0", %"class.std::initializer_list.0"* %[[AGG_RESULT]], i32 0, i32 0
+// CHECK: store i8** getelementptr inbounds ([2 x i8*], [2 x i8*]* @[[REFTMP]], i32 0, i32 0), i8*** %[[BEGIN]]
+// CHECK: %[[SIZE:.*]] = getelementptr inbounds %"class.std::initializer_list.0", %"class.std::initializer_list.0"* %[[AGG_RESULT]], i32 0, i32 1
+// CHECK: store i32 2, i32* %[[SIZE]]
+// CHECK: ret void
+
 void external();
 
 extern "C" void extended() {
   const auto && temporary = { [I new] };
   external();
 }
 
 // CHECK: [[INSTANCE:%.*]] = {{.*}} call i8* bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to i8* (i8*, i8*)*)(i8* {{.*}}, i8* {{.*}})
 // CHECK: {{.*}} call void @_Z8externalv()
 // CHECK: {{.*}} call void @objc_release(i8* {{.*}})
 
 std::initializer_list<I *> il = { [I new] };
 
 // CHECK: [[POOL:%.*]] = {{.*}} call i8* @objc_autoreleasePoolPush()
 // CHECK: [[INSTANCE:%.*]] = {{.*}} call i8* bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to i8* (i8*, i8*)*)(i8* {{.*}}, i8* {{.*}})
 // CHECK-NEXT: store i8* [[INSTANCE]], i8** bitcast ([1 x %0*]* @_ZGR2il_ to i8**)
 // CHECK: {{.*}} call void @objc_autoreleasePoolPop(i8* [[POOL]])
-