[CodeGen][ObjC] Fix assert on calling `__builtin_constant_p` with ObjC objects.

[vsapsai]

Summary

When we are calling __builtin_constant_p with ObjC objects of
different classes, we hit the assertion

Assertion failed: (isa<X>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file include/llvm/Support/Casting.h, line 254.

It happens because LLVM types for ObjCInterfaceType are opaque and
have no name (see CodeGenTypes::ConvertType). As the result, for
different ObjC classes we have different is_constant intrinsics with
the same name llvm.is.constant.p0s_s. When we try to reuse an
intrinsic with the same name, we fail because of type mismatch.

Fix by bitcasting ObjCObjectPointerType to id prior to passing as an
argument to __builtin_constant_p. This results in using intrinsic
llvm.is.constant.p0i8 and correct types.

rdar://problem/47499250

[void]

LGTM, but may want to wait for other reviewers.

[vsapsai]

In D57427#1376493, @void wrote:

LGTM, but may want to wait for other reviewers.

Thanks for the prompt review, Bill. I'll wait for other reviewers to confirm the change makes sense for Objective-C.

[ahatanak]

LGTM with one minor nit.

clang/test/CodeGenObjC/builtin-constant-p.m

1 ↗

(On Diff #184207)

Since this is just testing front-end's IRGen, can you pass -disable-llvm-passes ?

[vsapsai]

• Add in the test -disable-llvm-passes and relax FileCheck expectations to accept alloca-store-load.

[hans]

What's the status of this patch? Is it something we should merge onto the 8.0 release branch?

[vsapsai]

@ahatanak, can you please take a look again?

[vsapsai]

In D57427#1386781, @hans wrote:

What's the status of this patch? Is it something we should merge onto the 8.0 release branch?

Waiting on a one more review iteration from @ahatanak and hope that afterwards the patch should be good to go.

As for suitability for 8.0 release branch, I would ask @rjmccall for his opinion. As for me, I'm not entirely comfortable with CodeGen changes late in the release cycle. Also looks like in practice it shouldn't be a serious problem, __builtin_constant_p can be optimized out. For example, see https://godbolt.org/z/e1b4dB

[ahatanak]

LGTM

[hans]

Also looks like in practice it shouldn't be a serious problem, __builtin_constant_p can be optimized out. For example, see https://godbolt.org/z/e1b4dB

Okay, I'm not going to worry about it then. Thanks.

cfe/trunk/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_constant_p: {
 
     const Expr *Arg = E->getArg(0);
     QualType ArgType = Arg->getType();
     if (!hasScalarEvaluationKind(ArgType) || ArgType->isFunctionType())
       // We can only reason about scalar types.
       return RValue::get(ConstantInt::get(ResultType, 0));
 
     Value *ArgValue = EmitScalarExpr(Arg);
+    if (ArgType->isObjCObjectPointerType()) {
+      // Convert Objective-C objects to id because we cannot distinguish between
+      // LLVM types for Obj-C classes as they are opaque.
+      ArgType = CGM.getContext().getObjCIdType();
+      ArgValue = Builder.CreateBitCast(ArgValue, ConvertType(ArgType));
+    }
     Function *F =
         CGM.getIntrinsic(Intrinsic::is_constant, ConvertType(ArgType));
     Value *Result = Builder.CreateCall(F, ArgValue);
     if (Result->getType() != ResultType)
       Result = Builder.CreateIntCast(Result, ResultType, /*isSigned*/false);
     return RValue::get(Result);
   }
   case Builtin::BI__builtin_dynamic_object_size:

cfe/trunk/test/CodeGenObjC/builtin-constant-p.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -O3 -disable-llvm-passes -o - %s | FileCheck %s
+
+// Test that can call `__builtin_constant_p` with instances of different
+// Objective-C classes.
+// rdar://problem/47499250
+@class Foo;
+@class Bar;
+
+extern void callee(void);
+
+// CHECK-LABEL: define void @test(%0* %foo, %1* %bar)
+void test(Foo *foo, Bar *bar) {
+  // CHECK: [[ADDR_FOO:%.*]] = bitcast %0* %{{.*}} to i8*
+  // CHECK-NEXT: call i1 @llvm.is.constant.p0i8(i8* [[ADDR_FOO]])
+  // CHECK: [[ADDR_BAR:%.*]] = bitcast %1* %{{.*}} to i8*
+  // CHECK-NEXT: call i1 @llvm.is.constant.p0i8(i8* [[ADDR_BAR]])
+  if (__builtin_constant_p(foo) && __builtin_constant_p(bar))
+    callee();
+}
+
+// Test other Objective-C types.
+// CHECK-LABEL: define void @test_more(i8* %object, i8* %klass)
+void test_more(id object, Class klass) {
+  // CHECK: call i1 @llvm.is.constant.p0i8(i8* %{{.*}})
+  // CHECK: call i1 @llvm.is.constant.p0i8(i8* %{{.*}})
+  if (__builtin_constant_p(object) && __builtin_constant_p(klass))
+    callee();
+}