[CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC.

[vsapsai]

Summary

Failed assertion is

Assertion failed: ((ND->isUsed(false) || !isa<VarDecl>(ND) || !E->getLocation().isValid()) && "Should not use decl without marking it used!"), function EmitDeclRefLValue, file llvm-project/clang/lib/CodeGen/CGExpr.cpp, line 2437.

EmitDeclRefLValue mentions

A DeclRefExpr for a reference initialized by a constant expression can
appear without being odr-used. Directly emit the constant initializer.

The fix is in using the similar approach for non-references of
non-odr-used variables. EmitScalarExpr will try to emit constant if
possible and we can use resulting llvm::Value * without performing
EmitLValue.

rdar://problem/40650504

[rjmccall]

clang/lib/CodeGen/CGObjC.cpp

2480 ↗

(On Diff #170990)

This switch is just checking what you already computed as SuppressResultRetain. Please just assert in the second case that the qualifier is OCL_Weak.

Also, please stay consistent with the surrounding capitalization of local variables.

2527 ↗

(On Diff #170990)

Can we test constant-evaluability directly instead of only applying this when the declaration isn't otherwise used?

[vsapsai]

clang/lib/CodeGen/CGObjC.cpp

2480 ↗

(On Diff #170990)

Not entirely sure I understand the comment about switch correctly. Will change the code according to my understanding.

2527 ↗

(On Diff #170990)

We can use IsVariableAConstantExpression but it requires removing const from Decl.

Another way to test constant-evaluability is tryEmitAsConstant. It still requires removing const qualifier but overall it is a good approach. It captures the intention well and implementation-wise it is what CGF.EmitScalarExpr is doing anyway. I'll try it and will show how it looks like.

[vsapsai]

• Address review comments.

[vsapsai]

clang/lib/CodeGen/CGObjC.cpp

2527 ↗

(On Diff #170990)

If we go with tryEmitAsConstant, there is a separate review D53725 to use EmitConstant in more places.

[rjmccall]

clang/lib/CodeGen/CGObjC.cpp

2480 ↗

(On Diff #170990)

That's exactly what I was asking for, thank you.

[vsapsai]

• Rename EmitConstant to emitScalarConstant.
• Tweak comment to be explicitly about intended IR code, not about Obj-C++ code.

[vsapsai]

Exclude commits tracked in a different review.

[rjmccall]

LGTM.

[vsapsai]

Thanks for the review, John. I'll update the comment with TODO and commit.

cfe/trunk/lib/CodeGen/CGObjC.cpp

 
 void CodeGenFunction::EmitObjCAutoreleasePoolCleanup(llvm::Value *Ptr) {
   if (CGM.getLangOpts().ObjCAutoRefCount)
     EHStack.pushCleanup<CallObjCAutoreleasePoolObject>(NormalCleanup, Ptr);
   else
     EHStack.pushCleanup<CallObjCMRRAutoreleasePoolObject>(NormalCleanup, Ptr);
 }
 
-static TryEmitResult tryEmitARCRetainLoadOfScalar(CodeGenFunction &CGF,
-                                                  LValue lvalue,
-                                                  QualType type) {
-  switch (type.getObjCLifetime()) {
+static bool shouldRetainObjCLifetime(Qualifiers::ObjCLifetime lifetime) {
+  switch (lifetime) {
   case Qualifiers::OCL_None:
   case Qualifiers::OCL_ExplicitNone:
   case Qualifiers::OCL_Strong:
   case Qualifiers::OCL_Autoreleasing:
-    return TryEmitResult(CGF.EmitLoadOfLValue(lvalue,
-                                              SourceLocation()).getScalarVal(),
-                         false);
+    return true;
 
   case Qualifiers::OCL_Weak:
-    return TryEmitResult(CGF.EmitARCLoadWeakRetained(lvalue.getAddress()),
-                         true);
+    return false;
   }
 
   llvm_unreachable("impossible lifetime!");
 }
 
 static TryEmitResult tryEmitARCRetainLoadOfScalar(CodeGenFunction &CGF,
+                                                  LValue lvalue,
+                                                  QualType type) {
+  llvm::Value *result;
+  bool shouldRetain = shouldRetainObjCLifetime(type.getObjCLifetime());
+  if (shouldRetain) {
+    result = CGF.EmitLoadOfLValue(lvalue, SourceLocation()).getScalarVal();
+  } else {
+    assert(type.getObjCLifetime() == Qualifiers::OCL_Weak);
+    result = CGF.EmitARCLoadWeakRetained(lvalue.getAddress());
+  }
+  return TryEmitResult(result, !shouldRetain);
+}
+
+static TryEmitResult tryEmitARCRetainLoadOfScalar(CodeGenFunction &CGF,
                                                   const Expr *e) {
   e = e->IgnoreParens();
   QualType type = e->getType();
 
   // If we're loading retained from a __strong xvalue, we can avoid
   // an extra retain/release pair by zeroing out the source of this
   // "move" operation.
   if (e->isXValue() &&
static TryEmitResult tryEmitARCRetainLoadOfScalar(CodeGenFunction &CGF,
   // result of the call to objc_storeWeak instead of reloading.
   if (CGF.getLangOpts().CPlusPlus &&
       !type.isVolatileQualified() &&
       type.getObjCLifetime() == Qualifiers::OCL_Weak &&
       isa<BinaryOperator>(e) &&
       cast<BinaryOperator>(e)->getOpcode() == BO_Assign)
     return TryEmitResult(CGF.EmitScalarExpr(e), false);
 
+  // Try to emit code for scalar constant instead of emitting LValue and
+  // loading it because we are not guaranteed to have an l-value. One of such
+  // cases is DeclRefExpr referencing non-odr-used constant-evaluated variable.
+  if (const auto *decl_expr = dyn_cast<DeclRefExpr>(e)) {
+    auto *DRE = const_cast<DeclRefExpr *>(decl_expr);
+    if (CodeGenFunction::ConstantEmission constant = CGF.tryEmitAsConstant(DRE))
+      return TryEmitResult(CGF.emitScalarConstant(constant, DRE),
+                           !shouldRetainObjCLifetime(type.getObjCLifetime()));
+  }
+
   return tryEmitARCRetainLoadOfScalar(CGF, CGF.EmitLValue(e), type);
 }
 
 typedef llvm::function_ref<llvm::Value *(CodeGenFunction &CGF,
                                          llvm::Value *value)>
   ValueTransform;
 
 /// Insert code immediately after a call.

cfe/trunk/test/CodeGenObjCXX/arc-constexpr.mm

-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fobjc-arc -o - -std=c++11 %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fobjc-arc -fobjc-runtime-has-weak -o - -std=c++11 %s | FileCheck %s
 
 // CHECK: %[[TYPE:[a-z0-9]+]] = type opaque
 // CHECK: @[[CFSTRING:[a-z0-9_]+]] = private global %struct.__NSConstantString_tag
+@class NSString;
 
-// CHECK: define void @_Z5test1v
+// CHECK-LABEL: define void @_Z5test1v
 // CHECK:   %[[ALLOCA:[A-Z]+]] = alloca %[[TYPE]]*
 // CHECK:   %[[V0:[0-9]+]] = call i8* @objc_retain(i8* bitcast (%struct.__NSConstantString_tag* @[[CFSTRING]]
 // CHECK:   %[[V1:[0-9]+]] = bitcast i8* %[[V0]] to %[[TYPE]]*
 // CHECK:   store %[[TYPE]]* %[[V1]], %[[TYPE]]** %[[ALLOCA]]
 // CHECK:   %[[V2:[0-9]+]] = bitcast %[[TYPE]]** %[[ALLOCA]]
 // CHECK:   call void @objc_storeStrong(i8** %[[V2]], i8* null)
-
-@class NSString;
-
 void test1() {
   constexpr NSString *S = @"abc";
 }
+
+// CHECK-LABEL: define void @_Z5test2v
+// CHECK:      %[[CONST:[a-zA-Z]+]] = alloca %[[TYPE]]*
+// CHECK:      %[[REF_CONST:[a-zA-Z]+]] = alloca %[[TYPE]]*
+// CHECK:      %[[V0:[0-9]+]] = call i8* @objc_retain(i8* bitcast (%struct.__NSConstantString_tag* @[[CFSTRING]]
+// CHECK-NEXT: %[[V1:[0-9]+]] = bitcast i8* %[[V0]] to %[[TYPE]]*
+// CHECK-NEXT: store %[[TYPE]]* %[[V1]], %[[TYPE]]** %[[CONST]]
+// CHECK:      %[[V2:[0-9]+]] = call i8* @objc_retain(i8* bitcast (%struct.__NSConstantString_tag* @[[CFSTRING]]
+// CHECK-NEXT: %[[V3:[0-9]+]] = bitcast i8* %[[V2]] to %[[TYPE]]*
+// CHECK-NEXT: store %[[TYPE]]* %[[V3]], %[[TYPE]]** %[[REF_CONST]]
+// CHECK:      %[[V4:[0-9]+]] = bitcast %[[TYPE]]** %[[REF_CONST]]
+// CHECK-NEXT: call void @objc_storeStrong(i8** %[[V4]], i8* null)
+// CHECK:      %[[V5:[0-9]+]] = bitcast %[[TYPE]]** %[[CONST]]
+// CHECK-NEXT: call void @objc_storeStrong(i8** %[[V5]], i8* null)
+void test2() {
+  constexpr NSString *Const = @"abc";
+  // In IR RefConst should be initialized with Const initializer instead of
+  // reading from variable.
+  NSString* RefConst = Const;
+}
+
+// CHECK-LABEL: define void @_Z5test3v
+// CHECK:      %[[WEAK_CONST:[a-zA-Z]+]] = alloca %[[TYPE]]*
+// CHECK:      %[[REF_WEAK_CONST:[a-zA-Z]+]] = alloca %[[TYPE]]*
+// CHECK:      %[[V0:[0-9]+]] = bitcast %[[TYPE]]** %[[WEAK_CONST]]
+// CHECK-NEXT: %[[V1:[0-9]+]] = call i8* @objc_initWeak(i8** %[[V0]], i8* bitcast (%struct.__NSConstantString_tag* @[[CFSTRING]]
+// CHECK:      store %[[TYPE]]* bitcast (%struct.__NSConstantString_tag* @[[CFSTRING]] to %[[TYPE]]*), %[[TYPE]]** %[[REF_WEAK_CONST]]
+// CHECK:      %[[V2:[0-9]+]] = bitcast %[[TYPE]]** %[[REF_WEAK_CONST]]
+// CHECK-NEXT: call void @objc_storeStrong(i8** %[[V2]], i8* null)
+// CHECK:      %[[V3:[0-9]+]] = bitcast %[[TYPE]]** %[[WEAK_CONST]]
+// CHECK-NEXT: call void @objc_destroyWeak(i8** %[[V3]])
+void test3() {
+  __weak constexpr NSString *WeakConst = @"abc";
+  NSString* RefWeakConst = WeakConst;
+}