[OpenCL] Re-fix invalid address space generation for clk_event_t arguments of enqueue_kernel builtin function

[AlexeySotkin]

Summary

https://reviews.llvm.org/D53809 fixed wrong address space(assert in debug build)
generated for event_ret argument. But exactly the same problem exists for
event_wait_list argument. This patch should fix both.

[Anastasia]

lib/CodeGen/CGBuiltin.cpp

3711 ↗

(On Diff #192791)

It seems we are not testing the casts?

[AlexeySotkin]

lib/CodeGen/CGBuiltin.cpp

3711 ↗

(On Diff #192791)

Do you mean that when we run LIT tests, this code is not executed? If so, in the modified test below, literal zeros are making clang to execute CreateIntToPtr call indeed.
Or, do you mean that we need some extra check(to make sure the cast will be successful for example) in the source code itself ?

[Anastasia]

lib/CodeGen/CGBuiltin.cpp

3711 ↗

(On Diff #192791)

I mean since you are generating extra IR nodes we should check in the tests that they appear correctly. I don't see these casts checked in the tests currently.

[AlexeySotkin]

lib/CodeGen/CGBuiltin.cpp

3711 ↗

(On Diff #192791)

Since we are casting null constants they are folded to null values, like this %opencl.clk_event_t{{.*}}* addrspace(4)* null.

[AlexeySotkin]

lib/CodeGen/CGBuiltin.cpp

3711 ↗

(On Diff #192791)

I think 0 is the only possible integral literal, which can be given as the events arguments.

[AlexeySotkin]

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

[Anastasia]

In D59985#1453312, @AlexeySotkin wrote:

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

[AlexeySotkin]

In D59985#1454629, @Anastasia wrote:

In D59985#1453312, @AlexeySotkin wrote:

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

It must be rejected by SemaOpenCLBuiltinEnqueueKernel and it is already done https://godbolt.org/z/MFN3VU

[Anastasia]

In D59985#1454660, @AlexeySotkin wrote:

In D59985#1454629, @Anastasia wrote:

In D59985#1453312, @AlexeySotkin wrote:

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

It must be rejected by SemaOpenCLBuiltinEnqueueKernel and it is already done https://godbolt.org/z/MFN3VU

Ok, cool. Perhaps this is indeed a cleaner approach then?

[AlexeySotkin]

In D59985#1454676, @Anastasia wrote:

In D59985#1454660, @AlexeySotkin wrote:

In D59985#1454629, @Anastasia wrote:

In D59985#1453312, @AlexeySotkin wrote:

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

It must be rejected by SemaOpenCLBuiltinEnqueueKernel and it is already done https://godbolt.org/z/MFN3VU

Ok, cool. Perhaps this is indeed a cleaner approach then?

Ok, I'll update the patch.

[AlexeySotkin]

The patch has been updated. Now it checks if the argument isNullPointerConstant and emits null pointer directly in this case.

[Anastasia]

LGTM! Great! Thanks!

lib/CodeGen/CGBuiltin.cpp

3709 ↗

(On Diff #194098)

Just a minor issue of style, you could probably use a common lambda helper here. Although it's only used in this two places so definitely not that critical. It might be helpful mainly if we need to fix something later...

cfe/trunk/lib/CodeGen/CGBuiltin.cpp

if (E->getArg(3)->getType()->isBlockPointerType()) {
                                          llvm::ArrayRef<llvm::Value *>(Args)));
       if (TmpSize)
         EmitLifetimeEnd(TmpSize, TmpPtr);
       return Call;
     }
     // Any calls now have event arguments passed.
     if (NumArgs >= 7) {
       llvm::Type *EventTy = ConvertType(getContext().OCLClkEventTy);
-      llvm::Type *EventPtrTy = EventTy->getPointerTo(
+      llvm::PointerType *EventPtrTy = EventTy->getPointerTo(
           CGM.getContext().getTargetAddressSpace(LangAS::opencl_generic));
 
       llvm::Value *NumEvents =
           Builder.CreateZExtOrTrunc(EmitScalarExpr(E->getArg(3)), Int32Ty);
-      llvm::Value *EventList =
-          E->getArg(4)->getType()->isArrayType()
+
+      // Since SemaOpenCLBuiltinEnqueueKernel allows fifth and sixth arguments
+      // to be a null pointer constant (including `0` literal), we can take it
+      // into account and emit null pointer directly.
+      llvm::Value *EventWaitList = nullptr;
+      if (E->getArg(4)->isNullPointerConstant(
+              getContext(), Expr::NPC_ValueDependentIsNotNull)) {
+        EventWaitList = llvm::ConstantPointerNull::get(EventPtrTy);
+      } else {
+        EventWaitList = E->getArg(4)->getType()->isArrayType()
                         ? EmitArrayToPointerDecay(E->getArg(4)).getPointer()
                         : EmitScalarExpr(E->getArg(4));
-      llvm::Value *ClkEvent = EmitScalarExpr(E->getArg(5));
         // Convert to generic address space.
-      EventList = Builder.CreatePointerCast(EventList, EventPtrTy);
-      ClkEvent = ClkEvent->getType()->isIntegerTy()
-                   ? Builder.CreateBitOrPointerCast(ClkEvent, EventPtrTy)
-                   : Builder.CreatePointerCast(ClkEvent, EventPtrTy);
+        EventWaitList = Builder.CreatePointerCast(EventWaitList, EventPtrTy);
+      }
+      llvm::Value *EventRet = nullptr;
+      if (E->getArg(5)->isNullPointerConstant(
+              getContext(), Expr::NPC_ValueDependentIsNotNull)) {
+        EventRet = llvm::ConstantPointerNull::get(EventPtrTy);
+      } else {
+        EventRet =
+            Builder.CreatePointerCast(EmitScalarExpr(E->getArg(5)), EventPtrTy);
+      }
+
       auto Info =
           CGM.getOpenCLRuntime().emitOpenCLEnqueuedBlock(*this, E->getArg(6));
       llvm::Value *Kernel =
           Builder.CreatePointerCast(Info.Kernel, GenericVoidPtrTy);
       llvm::Value *Block =
           Builder.CreatePointerCast(Info.BlockArg, GenericVoidPtrTy);
 
       std::vector<llvm::Type *> ArgTys = {
           QueueTy,    Int32Ty,    RangeTy,          Int32Ty,
           EventPtrTy, EventPtrTy, GenericVoidPtrTy, GenericVoidPtrTy};
 
-      std::vector<llvm::Value *> Args = {Queue,     Flags,    Range,  NumEvents,
-                                         EventList, ClkEvent, Kernel, Block};
+      std::vector<llvm::Value *> Args = {Queue,     Flags,         Range,
+                                         NumEvents, EventWaitList, EventRet,
+                                         Kernel,    Block};
 
       if (NumArgs == 7) {
         // Has events but no variadics.
         Name = "__enqueue_kernel_basic_events";
         llvm::FunctionType *FTy = llvm::FunctionType::get(
             Int32Ty, llvm::ArrayRef<llvm::Type *>(ArgTys), false);
         return RValue::get(
             Builder.CreateCall(CGM.CreateRuntimeFunction(FTy, Name),

cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl

kernel void device_side_enqueue(global int *a, global int *b, int i) {
   // COMMON-SAME: i8 addrspace(4)* addrspacecast (i8* bitcast ({{.*}} [[INVLK2:[^ ]+_kernel]] to i8*) to i8 addrspace(4)*),
   // COMMON-SAME: i8 addrspace(4)* [[BL_I8]])
   enqueue_kernel(default_queue, flags, ndrange, 2, &event_wait_list, &clk_event,
                  ^(void) {
                    a[i] = b[i];
                  });
 
   // COMMON-LABEL: call i32 @__enqueue_kernel_basic_events
-  // COMMON-SAME: (%opencl.queue_t{{.*}}* {{%[0-9]+}}, i32 {{%[0-9]+}}, %struct.ndrange_t* {{.*}}, i32 1, %opencl.clk_event_t{{.*}}* addrspace(4)* {{%[0-9]+}}, %opencl.clk_event_t{{.*}}* addrspace(4)* null,
-  enqueue_kernel(default_queue, flags, ndrange, 1, &event_wait_list, 0,
+  // COMMON-SAME: (%opencl.queue_t{{.*}}* {{%[0-9]+}}, i32 {{%[0-9]+}}, %struct.ndrange_t* {{.*}}, i32 1, %opencl.clk_event_t{{.*}}* addrspace(4)* null, %opencl.clk_event_t{{.*}}* addrspace(4)* null,
+  enqueue_kernel(default_queue, flags, ndrange, 1, 0, 0,
                  ^(void) {
                    return;
                  });
 
   // Emits global block literal [[BLG1]] and block kernel [[INVGK1]].
   // COMMON: [[DEF_Q:%[0-9]+]] = load %opencl.queue_t{{.*}}*, %opencl.queue_t{{.*}}** %default_queue
   // COMMON: [[FLAGS:%[0-9]+]] = load i32, i32* %flags
   // CHECK-LIFETIMES: [[LIFETIME_PTR:%[0-9]+]] = bitcast [1 x i64]* %[[BLOCK_SIZES1]] to i8*