Derive builtin return type from its definition

[mantognini]

Summary

Prior to this patch, OpenCL code such as the following would attempt to create
a BranchInst with a non-bool argument:

if (enqueue_kernel(get_default_queue(), 0, nd, ^(void){})) /* ... */

This patch is a follow up on a similar issue with pipe builtin
operations. See commit r280800 and https://bugs.llvm.org/show_bug.cgi?id=30219.

This change, while being conservative on non-builtin functions,
should set the type of expressions invoking builtins to the
proper type, instead of defaulting to bool and requiring
manual overrides in Sema::CheckBuiltinFunctionCall.

In addition to tests for enqueue_kernel, the tests are extended to
check other OpenCL builtins.

[Anastasia]

LGTM from OpenCL side!

[mantognini]

ping

[Anastasia]

LGTM! It looks reasonable small change!

[riccibruno]

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

[mantognini]

In D52879#1309734, @riccibruno wrote:

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

Mainly because I was focusing on OpenCL builtins and was confident about changing those, but wanted to be conservative on code that I don't test/work with. If one wanted to remove those, I would encourage them to double check their definitions are correct as I had to fix (https://reviews.llvm.org/D52875) some for OpenCL.

[riccibruno]

In D52879#1309856, @mantognini wrote:

In D52879#1309734, @riccibruno wrote:

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

Mainly because I was focusing on OpenCL builtins and was confident about changing those, but wanted to be conservative on code that I don't test/work with. If one wanted to remove those, I would encourage them to double check their definitions are correct as I had to fix (https://reviews.llvm.org/D52875) some for OpenCL.

Got it. Thanks for explanation. Perhaps you could leave a TODO here with a note including what you just said so that
we don't forget about it in the future (including the advice to double check the definition) ?

[riccibruno]

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

[riccibruno]

In D52879#1311177, @riccibruno wrote:

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

@mantognini Ping ? I agree with you that it would be best to derive the return type of builtins from
their definition, but as such I think that this change is incorrect for the reason given above.

What I would like to propose is to revert the changes to the code in Sema, but keep the tests.

[mantognini]

In D52879#1311177, @riccibruno wrote:

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

Could you clarify one thing for me: If I understood what you were saying, the type passed to CallExpr should not be ref-cv-qualified, is that right?

In that case, couldn't we "simply" remove the ref + CV-qualifiers from ReturnTy? (getNonReferenceType().withoutLocalFastQualifiers())

Note: I cannot revert this and keep the test because the new ones will fail.

[riccibruno]

In D52879#1314694, @mantognini wrote:

In D52879#1311177, @riccibruno wrote:

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

Could you clarify one thing for me: If I understood what you were saying, the type passed to CallExpr should not be ref-cv-qualified, is that right?

In that case, couldn't we "simply" remove the ref + CV-qualifiers from ReturnTy? (getNonReferenceType().withoutLocalFastQualifiers())

Note: I cannot revert this and keep the test because the new ones will fail.

It's not that simple unfortunately. The type passed to CallExpr is the type of the call expression,
which is different from the return type. However you can just use FunctionType::getCallResultType.

lib/Sema/SemaExpr.cpp

5550

Please rename this to something more appropriate,
like ResultTy or similar.

5553

Please leave a FIXME indicating that someone should
go through each of the builtins and remove the
various setType when appropriate.

5554

Don't use auto here.

5556

const auto * or just spell the type.

[mantognini]

Thank you for the detailed review. I'll work on a patch and add you as reviewer once done (prob. on Monday though).

[riccibruno]

In D52879#1314855, @mantognini wrote:

Thank you for the detailed review. I'll work on a patch and add you as reviewer once done (prob. on Monday though).

That's fine. Thanks for your contribution !

[mantognini]

Please see https://reviews.llvm.org/D55136 for the patch addressing these issues.

lib/Sema/SemaChecking.cpp

case Builtin::BI__GetExceptionInfo:
     break;
   // OpenCL v2.0, s6.13.16 - Pipe functions
   case Builtin::BIread_pipe:
   case Builtin::BIwrite_pipe:
     // Since those two functions are declared with var args, we need a semantic
     // check for the argument.
     if (SemaBuiltinRWPipe(*this, TheCall))
       return ExprError();
-    TheCall->setType(Context.IntTy);
     break;
   case Builtin::BIreserve_read_pipe:
   case Builtin::BIreserve_write_pipe:
   case Builtin::BIwork_group_reserve_read_pipe:
   case Builtin::BIwork_group_reserve_write_pipe:
     if (SemaBuiltinReserveRWPipe(*this, TheCall))
       return ExprError();
     break;
case Builtin::BIsub_group_commit_write_pipe:
     if (checkOpenCLSubgroupExt(*this, TheCall) ||
         SemaBuiltinCommitRWPipe(*this, TheCall))
       return ExprError();
     break;
   case Builtin::BIget_pipe_num_packets:
   case Builtin::BIget_pipe_max_packets:
     if (SemaBuiltinPipePackets(*this, TheCall))
       return ExprError();
-    TheCall->setType(Context.UnsignedIntTy);
     break;
   case Builtin::BIto_global:
   case Builtin::BIto_local:
   case Builtin::BIto_private:
     if (SemaOpenCLBuiltinToAddr(*this, BuiltinID, TheCall))
       return ExprError();
     break;
   // OpenCL v2.0, s6.13.17 - Enqueue kernel functions.

lib/Sema/SemaExpr.cpp

if (Caller->hasAttr<ARMInterruptAttr>()) {
       if (VFP && (!FDecl || !FDecl->hasAttr<ARMInterruptAttr>()))
         Diag(Fn->getExprLoc(), diag::warn_arm_interrupt_calling_convention);
     }
 
   // Promote the function operand.
   // We special-case function promotion here because we only allow promoting
   // builtin functions to function pointers in the callee of a call.
   ExprResult Result;
+  QualType ReturnTy;
   if (BuiltinID &&
       Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
-    Result = ImpCastExprToType(Fn, Context.getPointerType(FDecl->getType()),
-                               CK_BuiltinFnToFnPtr).get();
+    // Extract the return type from the (builtin) function pointer type.
+    auto FnPtrTy = Context.getPointerType(FDecl->getType());
+    Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
+    auto FnTy = FnPtrTy->getPointeeType()->castAs<FunctionType>();
+    ReturnTy = FnTy->getReturnType();
   } else {
     Result = CallExprUnaryConversions(Fn);
+    ReturnTy = Context.BoolTy;
   }
   if (Result.isInvalid())
     return ExprError();
   Fn = Result.get();
 
   // Make the call expr early, before semantic checks.  This guarantees cleanup
   // of arguments and function on error.
   CallExpr *TheCall;
   if (Config)
-    TheCall = new (Context) CUDAKernelCallExpr(Context, Fn,
-                                               cast<CallExpr>(Config), Args,
-                                               Context.BoolTy, VK_RValue,
-                                               RParenLoc);
+    TheCall =
+        new (Context) CUDAKernelCallExpr(Context, Fn, cast<CallExpr>(Config),
+                                         Args, ReturnTy, VK_RValue, RParenLoc);
   else
-    TheCall = new (Context) CallExpr(Context, Fn, Args, Context.BoolTy,
-                                     VK_RValue, RParenLoc);
+    TheCall = new (Context)
+        CallExpr(Context, Fn, Args, ReturnTy, VK_RValue, RParenLoc);
 
   if (!getLangOpts().CPlusPlus) {
     // C cannot always handle TypoExpr nodes in builtin calls and direct
     // function calls as their argument checking don't necessarily handle
     // dependent types properly, so make sure any TypoExprs have been
     // dealt with.
     ExprResult Result = CorrectDelayedTyposInExpr(TheCall);
     if (!Result.isUsable()) return ExprError();

test/CodeGenOpenCL/builtins.cl

+// RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
+
+void testBranchingOnEnqueueKernel(queue_t default_queue, unsigned flags, ndrange_t ndrange) {
+    // Ensure `enqueue_kernel` can be branched upon.
+
+    if (enqueue_kernel(default_queue, flags, ndrange, ^(void) {}))
+        (void)0;
+    // CHECK: [[P:%[0-9]+]] = call i32 @__enqueue_kernel
+    // CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+    // CHECK-NEXT: br i1 [[Q]]
+
+    if (get_kernel_work_group_size(^(void) {}))
+        (void)0;
+    // CHECK: [[P:%[0-9]+]] = call i32 @__get_kernel_work_group_size
+    // CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+    // CHECK-NEXT: br i1 [[Q]]
+
+    if (get_kernel_preferred_work_group_size_multiple(^(void) {}))
+        (void)0;
+    // CHECK: [[P:%[0-9]+]] = call i32 @__get_kernel_preferred_work_group_size_multiple_impl
+    // CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+    // CHECK-NEXT: br i1 [[Q]]
+}
+
+void testBranchinOnPipeOperations(read_only pipe int r, write_only pipe int w, global int* ptr) {
+    // Verify that return type is correctly casted to i1 value.
+
+    if (read_pipe(r, ptr))
+        (void)0;
+    // CHECK: [[R:%[0-9]+]] = call i32 @__read_pipe_2
+    // CHECK-NEXT: icmp ne i32 [[R]], 0
+
+    if (write_pipe(w, ptr))
+        (void)0;
+    // CHECK: [[R:%[0-9]+]] = call i32 @__write_pipe_2
+    // CHECK-NEXT: icmp ne i32 [[R]], 0
+
+    if (get_pipe_num_packets(r))
+        (void)0;
+    // CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_num_packets_ro
+    // CHECK-NEXT: icmp ne i32 [[R]], 0
+
+    if (get_pipe_num_packets(w))
+        (void)0;
+    // CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_num_packets_wo
+    // CHECK-NEXT: icmp ne i32 [[R]], 0
+
+    if (get_pipe_max_packets(r))
+        (void)0;
+    // CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_max_packets_ro
+    // CHECK-NEXT: icmp ne i32 [[R]], 0
+
+    if (get_pipe_max_packets(w))
+        (void)0;
+    // CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_max_packets_wo
+    // CHECK-NEXT: icmp ne i32 [[R]], 0
+}
+
+void testBranchingOnAddressSpaceCast(generic long* ptr) {
+    // Verify that pointer types are properly casted, respecting address spaces.
+
+    if (to_global(ptr))
+        (void)0;
+    // CHECK:       [[P:%[0-9]+]] = call [[GLOBAL_VOID:i8 addrspace\(1\)\*]] @__to_global([[GENERIC_VOID:i8 addrspace\(4\)\*]] {{%[0-9]+}})
+    // CHECK-NEXT:  [[Q:%[0-9]+]] = bitcast [[GLOBAL_VOID]] [[P]] to [[GLOBAL_i64:i64 addrspace\(1\)\*]]
+    // CHECK-NEXT:  [[BOOL:%[a-z0-9]+]] = icmp ne [[GLOBAL_i64]] [[Q]], null
+    // CHECK-NEXT:  br i1 [[BOOL]]
+
+    if (to_local(ptr))
+        (void)0;
+    // CHECK:       [[P:%[0-9]+]] = call [[LOCAL_VOID:i8 addrspace\(3\)\*]] @__to_local([[GENERIC_VOID]] {{%[0-9]+}})
+    // CHECK-NEXT:  [[Q:%[0-9]+]] = bitcast [[LOCAL_VOID]] [[P]] to [[LOCAL_i64:i64 addrspace\(3\)\*]]
+    // CHECK-NEXT:  [[BOOL:%[a-z0-9]+]] = icmp ne [[LOCAL_i64]] [[Q]], null
+    // CHECK-NEXT:  br i1 [[BOOL]]
+
+    if (to_private(ptr))
+        (void)0;
+    // CHECK:       [[P:%[0-9]+]] = call [[PRIVATE_VOID:i8\*]] @__to_private([[GENERIC_VOID]] {{%[0-9]+}})
+    // CHECK-NEXT:  [[Q:%[0-9]+]] = bitcast [[PRIVATE_VOID]] [[P]] to [[PRIVATE_i64:i64\*]]
+    // CHECK-NEXT:  [[BOOL:%[a-z0-9]+]] = icmp ne [[PRIVATE_i64]] [[Q]], null
+    // CHECK-NEXT:  br i1 [[BOOL]]
+}
+

test/CodeGenOpenCL/pipe_builtin.cl

 }
 
 void test8(write_only pipe int p, global int *ptr) {
   // CHECK: call i32 @__get_pipe_num_packets_wo(%opencl.pipe_wo_t* %{{.*}}, i32 4, i32 4)
   *ptr = get_pipe_num_packets(p);
   // CHECK: call i32 @__get_pipe_max_packets_wo(%opencl.pipe_wo_t* %{{.*}}, i32 4, i32 4)
   *ptr = get_pipe_max_packets(p);
 }
-
-void test9(read_only pipe int r, write_only pipe int w, global int *ptr) {
-  // verify that return type is correctly casted to i1 value
-  // CHECK: %[[R:[0-9]+]] = call i32 @__read_pipe_2
-  // CHECK: icmp ne i32 %[[R]], 0
-  if (read_pipe(r, ptr)) *ptr = -1;
-  // CHECK: %[[W:[0-9]+]] = call i32 @__write_pipe_2
-  // CHECK: icmp ne i32 %[[W]], 0
-  if (write_pipe(w, ptr)) *ptr = -1;
-  // CHECK: %[[NR:[0-9]+]] = call i32 @__get_pipe_num_packets_ro
-  // CHECK: icmp ne i32 %[[NR]], 0
-  if (get_pipe_num_packets(r)) *ptr = -1;
-  // CHECK: %[[NW:[0-9]+]] = call i32 @__get_pipe_num_packets_wo
-  // CHECK: icmp ne i32 %[[NW]], 0
-  if (get_pipe_num_packets(w)) *ptr = -1;
-  // CHECK: %[[MR:[0-9]+]] = call i32 @__get_pipe_max_packets_ro
-  // CHECK: icmp ne i32 %[[MR]], 0
-  if (get_pipe_max_packets(r)) *ptr = -1;
-  // CHECK: %[[MW:[0-9]+]] = call i32 @__get_pipe_max_packets_wo
-  // CHECK: icmp ne i32 %[[MW]], 0
-  if (get_pipe_max_packets(w)) *ptr = -1;
-}