[OpenCL] Set external linkage for block enqueue kernels

[svenvh]

Summary

All kernels can be called from the host as per the SPIR_KERNEL calling
convention. As such, all kernels should have external linkage, but
block enqueue kernels were created with internal linkage.

Reported-by: Pedro Olsen Ferreira

[Anastasia]

LGTM! I agree according to the OpenCL programming model attributes internal spir_kernel together on the same function create a contradiction because kernel functions are expected to have global visibility.

Adding @AlexeySachkov who has reported the same issue earlier elsewhere.

[yaxunl]

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

[Anastasia]

In D115523#3237410, @yaxunl wrote:

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

Potentially we should append the name of the translation unit to all kernel wrapper names for the enqueued blocks to resolve this? For example, global constructors stubs are using such a similar naming scheme taken from the translation unit.

But the kernel function in OpenCL has to be globally visible and many tools have been built with this assumption. Additionally, some toolchains might require the enqueued kernels to be globally visible as well in order to access them as an execution entry point.

[yaxunl]

In D115523#3237857, @Anastasia wrote:

In D115523#3237410, @yaxunl wrote:

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

Potentially we should append the name of the translation unit to all kernel wrapper names for the enqueued blocks to resolve this? For example, global constructors stubs are using such a similar naming scheme taken from the translation unit.

But the kernel function in OpenCL has to be globally visible and many tools have been built with this assumption. Additionally, some toolchains might require the enqueued kernels to be globally visible as well in order to access them as an execution entry point.

If we have to externalize such block kernels whose names are derived from variables with internal linkage, we may need a way to make the block kernel names unique.

One approach would be use MD5 hash of the file path and compile options, similar to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L2623

[Anastasia]

In D115523#3240870, @yaxunl wrote:

In D115523#3237857, @Anastasia wrote:

In D115523#3237410, @yaxunl wrote:

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

Potentially we should append the name of the translation unit to all kernel wrapper names for the enqueued blocks to resolve this? For example, global constructors stubs are using such a similar naming scheme taken from the translation unit.

But the kernel function in OpenCL has to be globally visible and many tools have been built with this assumption. Additionally, some toolchains might require the enqueued kernels to be globally visible as well in order to access them as an execution entry point.

If we have to externalize such block kernels whose names are derived from variables with internal linkage, we may need a way to make the block kernel names unique.

One approach would be use MD5 hash of the file path and compile options, similar to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L2623

Ok, however it might be enough to do what C++ handling does with global ctors. Should we file a bug for this for now?

[yaxunl]

In D115523#3266584, @Anastasia wrote:

In D115523#3240870, @yaxunl wrote:

In D115523#3237857, @Anastasia wrote:

In D115523#3237410, @yaxunl wrote:

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

Potentially we should append the name of the translation unit to all kernel wrapper names for the enqueued blocks to resolve this? For example, global constructors stubs are using such a similar naming scheme taken from the translation unit.

But the kernel function in OpenCL has to be globally visible and many tools have been built with this assumption. Additionally, some toolchains might require the enqueued kernels to be globally visible as well in order to access them as an execution entry point.

If we have to externalize such block kernels whose names are derived from variables with internal linkage, we may need a way to make the block kernel names unique.

One approach would be use MD5 hash of the file path and compile options, similar to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L2623

Ok, however it might be enough to do what C++ handling does with global ctors. Should we file a bug for this for now?

Do you mean the global ctors like this? https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenCXX/global-init.cpp#L201

They do not have unique names across TU's. They work because they have internal linkage. However OpenCL kernels have external linkage.

I suggest opening a bug to track this. Thanks.

[Anastasia]

In D115523#3284446, @yaxunl wrote:

In D115523#3266584, @Anastasia wrote:

In D115523#3240870, @yaxunl wrote:

In D115523#3237857, @Anastasia wrote:

In D115523#3237410, @yaxunl wrote:

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

Potentially we should append the name of the translation unit to all kernel wrapper names for the enqueued blocks to resolve this? For example, global constructors stubs are using such a similar naming scheme taken from the translation unit.

But the kernel function in OpenCL has to be globally visible and many tools have been built with this assumption. Additionally, some toolchains might require the enqueued kernels to be globally visible as well in order to access them as an execution entry point.

If we have to externalize such block kernels whose names are derived from variables with internal linkage, we may need a way to make the block kernel names unique.

One approach would be use MD5 hash of the file path and compile options, similar to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L2623

Ok, however it might be enough to do what C++ handling does with global ctors. Should we file a bug for this for now?

Do you mean the global ctors like this? https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenCXX/global-init.cpp#L201

They do not have unique names across TU's. They work because they have internal linkage. However OpenCL kernels have external linkage.

I suggest opening a bug to track this. Thanks.

Sure: https://github.com/llvm/llvm-project/issues/53572. Feel free to add anything relevant.

clang/lib/CodeGen/TargetInfo.cpp

TargetCodeGenInfo::createEnqueuedBlockKernel(CodeGenFunction &CGF,
                                              llvm::Value *BlockLiteral) const {
   auto *InvokeFT = Invoke->getFunctionType();
   llvm::SmallVector<llvm::Type *, 2> ArgTys;
   for (auto &P : InvokeFT->params())
     ArgTys.push_back(P);
   auto &C = CGF.getLLVMContext();
   std::string Name = Invoke->getName().str() + "_kernel";
   auto *FT = llvm::FunctionType::get(llvm::Type::getVoidTy(C), ArgTys, false);
-  auto *F = llvm::Function::Create(FT, llvm::GlobalValue::InternalLinkage, Name,
+  auto *F = llvm::Function::Create(FT, llvm::GlobalValue::ExternalLinkage, Name,
                                    &CGF.CGM.getModule());
   auto IP = CGF.Builder.saveIP();
   auto *BB = llvm::BasicBlock::Create(C, "entry", F);
   auto &Builder = CGF.Builder;
   Builder.SetInsertPoint(BB);
   llvm::SmallVector<llvm::Value *, 2> Args;
   for (auto &A : F->args())
     Args.push_back(&A);

clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl

kernel void device_side_enqueue(global int *a, global int *b, int i) {
 
   // Emits global block literal [[BLG11]] and block kernel [[INVGK11]].
   // COMMON: call spir_func i32 @__get_kernel_sub_group_count_for_ndrange_impl(%struct.ndrange_t* {{[^,]+}},
   // COMMON-SAME: i8 addrspace(4)* addrspacecast (i8* bitcast ({{.*}} [[INVGK11:[^ ]+_kernel]] to i8*) to i8 addrspace(4)*),
   // COMMON-SAME: i8 addrspace(4)* addrspacecast (i8 addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BLG11]] to i8 addrspace(1)*) to i8 addrspace(4)*))
   size = get_kernel_sub_group_count_for_ndrange(ndrange, ^(){});
 }
 
-// COMMON: define internal spir_kernel void [[INVLK1]](i8 addrspace(4)* %0) #{{[0-9]+}} {
+// COMMON: define spir_kernel void [[INVLK1]](i8 addrspace(4)* %0) #{{[0-9]+}} {
 // COMMON: entry:
 // COMMON:  call spir_func void @__device_side_enqueue_block_invoke(i8 addrspace(4)* %0)
 // COMMON:  ret void
 // COMMON: }
-// COMMON: define internal spir_kernel void [[INVLK2]](i8 addrspace(4)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK1]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK2]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK3]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK4]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK5]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK6]](i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3) #{{[0-9]+}} {
+// COMMON: define spir_kernel void [[INVLK2]](i8 addrspace(4)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK1]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK2]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK3]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK4]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK5]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK6]](i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3) #{{[0-9]+}} {
 // COMMON: entry:
 // COMMON:  call spir_func void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
 // COMMON:  ret void
 // COMMON: }
-// COMMON: define internal spir_kernel void [[INVGK7]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK7]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
 // COMMON: define internal spir_func void [[INVG8]](i8 addrspace(4)*{{.*}})
 // COMMON: define internal spir_func void [[INVG9]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)* %{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK8]](i8 addrspace(4)*{{.*}})
-// COMMON: define internal spir_kernel void [[INV_G_K]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVLK3]](i8 addrspace(4)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK9]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK10]](i8 addrspace(4)*{{.*}})
-// COMMON: define internal spir_kernel void [[INVGK11]](i8 addrspace(4)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK8]](i8 addrspace(4)*{{.*}})
+// COMMON: define spir_kernel void [[INV_G_K]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVLK3]](i8 addrspace(4)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK9]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK10]](i8 addrspace(4)*{{.*}})
+// COMMON: define spir_kernel void [[INVGK11]](i8 addrspace(4)*{{.*}})