[OpenCL][ARM] Fix ICE when compiling a kernel

[Anastasia]

Summary

Compiling any OpenCL source for Arm CPU is pointless because when the kernel is used the compilation will fail due to unsupported calling convention SPIR_KERNEL.

The backend can't handle this and it probably doesn't make much sense to use SPIR-specific logic for Arm considering that it has been discontinued. At the same time, there is no special calling convention added for C++ threads. For now, I added regular C calling conversions by default for all OpenCL kernels in Arm CPU. This prevents the crash and allows to start using the functionality in some projects i.e. https://github.com/compiler-explorer/compiler-explorer/issues/2741.

In a long term, we might need to do something more than this to provide fully functional support.

Fixes PR50841!

[pekka.jaaskelainen]

Does this break clSetKernelArg() for ARM CPUs in PoCL? I believe so far it has worked for ARM - just wondering why you decide to drop it now?

https://github.com/pocl/pocl/issues/1

[Anastasia]

In D104858#2849268, @pekka.jaaskelainen wrote:

Does this break clSetKernelArg() for ARM CPUs in PoCL? I believe so far it has worked for ARM - just wondering why you decide to drop it now?

https://github.com/pocl/pocl/issues/1

The reason why we would like to fix it is that upstream clang has a crash currently when OpenCL sources are compiled for any Arm CPU: https://bugs.llvm.org/show_bug.cgi?id=50841. Do you have any other suggestions to avoid this problem?

FYI clang also emits kernel metadata that can be used to detect kernels...

[pekka.jaaskelainen]

The reason why we would like to fix it is that upstream clang has a crash currently when OpenCL sources are compiled for any Arm CPU: https://bugs.llvm.org/show_bug.cgi?id=50841. Do you have any other suggestions to avoid this problem?

FYI clang also emits kernel metadata that can be used to detect kernels...

Unfortunately I cannot look at this in detail right now, but I'll reply quickly for a heads up: The problem was not how to detect kernels, but the ABI/CC mismatch to the
clSetKerneArg() with (user facing) arguments that get split to multiple args or vice versa. E.g. x86 ABI had a CC where 2xfloat gets to 1xdouble in the generated function
finger print, thus there was no 1:1 match of the kernel arguments to the OpenCL-facing ones (making clSetKernelArg calls difficult to implement robustly).

IIRC, SPIR_KERNEL CC was used to force the 1:1 mapping and produce a portable way to handle this OpenCL API e.g. with struct args etc.

[Anastasia]

In D104858#2857958, @pekka.jaaskelainen wrote:

The reason why we would like to fix it is that upstream clang has a crash currently when OpenCL sources are compiled for any Arm CPU: https://bugs.llvm.org/show_bug.cgi?id=50841. Do you have any other suggestions to avoid this problem?

FYI clang also emits kernel metadata that can be used to detect kernels...

Unfortunately I cannot look at this in detail right now, but I'll reply quickly for a heads up: The problem was not how to detect kernels, but the ABI/CC mismatch to the
clSetKerneArg() with (user facing) arguments that get split to multiple args or vice versa. E.g. x86 ABI had a CC where 2xfloat gets to 1xdouble in the generated function
finger print, thus there was no 1:1 match of the kernel arguments to the OpenCL-facing ones (making clSetKernelArg calls difficult to implement robustly).

IIRC, SPIR_KERNEL CC was used to force the 1:1 mapping and produce a portable way to handle this OpenCL API e.g. with struct args etc.

Ok, thanks for clarification. Does it mean there is something we need to add to LLVM somewhere to make it work correctly? Would it be specific to Arm or generally for all CPU targets?

[pekka.jaaskelainen]

Ok, thanks for clarification. Does it mean there is something we need to add to LLVM somewhere to make it work correctly? Would it be specific to Arm or generally for all CPU targets?

To my understanding, what is required is to keep SPIR_KERNEL CC working for Arm since that is the generic way for PoCL supports clSetKernelArg for CPU targets by default. I have been under impression that it has worked so far for Arm, but I haven't tested it personally, and PoCL still lacks an official Arm maintainer, so cannot say for sure.

[Anastasia]

In D104858#2859742, @pekka.jaaskelainen wrote:

Ok, thanks for clarification. Does it mean there is something we need to add to LLVM somewhere to make it work correctly? Would it be specific to Arm or generally for all CPU targets?

To my understanding, what is required is to keep SPIR_KERNEL CC working for Arm since that is the generic way for PoCL supports clSetKernelArg for CPU targets by default. I have been under impression that it has worked so far for Arm, but I haven't tested it personally, and PoCL still lacks an official Arm maintainer, so cannot say for sure.

Ok, but as is clang can't compile OpenCL sources

In D104858#2859742, @pekka.jaaskelainen wrote:

Ok, thanks for clarification. Does it mean there is something we need to add to LLVM somewhere to make it work correctly? Would it be specific to Arm or generally for all CPU targets?

To my understanding, what is required is to keep SPIR_KERNEL CC working for Arm since that is the generic way for PoCL supports clSetKernelArg for CPU targets by default. I have been under impression that it has worked so far for Arm, but I haven't tested it personally, and PoCL still lacks an official Arm maintainer, so cannot say for sure.

Well right now clang doesn't seem to cope with SPIR_KERNEL for Arm CPU as it crashes its instruction selection https://godbolt.org/z/rraG9roee. So the flow is definitely broken. And if we fix the failure we can use this properly in godbolt instead of working around by removing the kernel in the preprocessor.

[stuart]

Unfortunately I cannot look at this in detail right now, but I'll reply quickly for a heads up: The problem was not how to detect kernels, but the ABI/CC mismatch to the clSetKerneArg() with (user facing) arguments that get split to multiple args or vice versa. E.g. x86 ABI had a CC where 2xfloat gets to 1xdouble in the generated function finger print, thus there was no 1:1 match of the kernel arguments to the OpenCL-facing ones (making clSetKernelArg calls difficult to implement robustly).

Could you not just use libffi?

clang/lib/CodeGen/TargetInfo.cpp

if (BPI.SignReturnAddr != LangOptions::SignReturnAddressScopeKind::None) {
                     BPI.SignKey == LangOptions::SignReturnAddressKeyKind::AKey
                         ? "a_key"
                         : "b_key");
     }
 
     Fn->addFnAttr("branch-target-enforcement",
                   BPI.BranchTargetEnforcement ? "true" : "false");
   }
+
+  unsigned getOpenCLKernelCallingConv() const override {
+    // OpenCL kernels use calling convention from the regular functions
+    // as there is no special support for threads.
+    return llvm::CallingConv::C;
+  }
 };
 
 class WindowsAArch64TargetCodeGenInfo : public AArch64TargetCodeGenInfo {
 public:
   WindowsAArch64TargetCodeGenInfo(CodeGenTypes &CGT, AArch64ABIInfo::ABIKind K)
       : AArch64TargetCodeGenInfo(CGT, K) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
 
     // AAPCS guarantees that sp will be 8-byte aligned on any public interface,
     // however this is not necessarily true on taking any interrupt. Instruct
     // the backend to perform a realignment as part of the function prologue.
     llvm::AttrBuilder B;
     B.addStackAlignmentAttr(8);
     Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
   }
+
+  unsigned getOpenCLKernelCallingConv() const override {
+    // OpenCL kernels use calling convention from the regular functions
+    // as there is no special support for threads.
+    return llvm::CallingConv::C;
+  }
 };
 
 class WindowsARMTargetCodeGenInfo : public ARMTargetCodeGenInfo {
 public:
   WindowsARMTargetCodeGenInfo(CodeGenTypes &CGT, ARMABIInfo::ABIKind K)
       : ARMTargetCodeGenInfo(CGT, K) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,

clang/test/CodeGenOpenCL/kernels-have-spir-cc-by-default.cl

 // RUN: %clang_cc1 %s -cl-std=CL1.2 -emit-llvm -triple x86_64-unknown-unknown -o - | FileCheck %s
 // RUN: %clang_cc1 %s -cl-std=CL1.2 -emit-llvm -triple amdgcn-unknown-unknown -o - | FileCheck -check-prefixes=AMDGCN %s
-// Test that the kernels always use the SPIR calling convention
-// to have unambiguous mapping of arguments to feasibly implement
+// RUN: %clang_cc1 %s -cl-std=CL1.2 -emit-llvm -triple arm -o - | FileCheck -check-prefixes=ARM %s
+// RUN: %clang_cc1 %s -cl-std=CL1.2 -emit-llvm -triple aarch64 -o - | FileCheck -check-prefixes=AARCH64 %s
+
+// Test that the kernels use the SPIR calling convention for targets that
+// support it.
+// This facilitates unambiguous mapping of arguments to feasibly implement
 // clSetKernelArg().
 
+// ARM-NOT: spir_kernel
+// AARCH64-NOT: spir_kernel
+
 typedef struct int_single {
     int a;
 } int_single;
 
 typedef struct int_pair {
     long a;
     long b;
 } int_pair;
 
 typedef struct test_struct {
     int elementA;
     int elementB;
     long elementC;
     char elementD;
     long elementE;
     float elementF;
     short elementG;
+// FIXME: double type should be enabled correctly for Arm CPU.
+#if !defined(__ARM_ARCH)
     double elementH;
+#endif
 } test_struct;
 
 kernel void test_single(int_single input, global int* output) {
 // CHECK: spir_kernel
 // AMDGCN: define{{.*}} amdgpu_kernel void @test_single
 // CHECK: struct.int_single* nocapture {{.*}} byval(%struct.int_single)
 // CHECK: i32* nocapture %output
  output[0] = input.a;

 // CHECK: i32* nocapture %output
  output[0] = input.elementA;
  output[1] = input.elementB;
  output[2] = (int)input.elementC;
  output[3] = (int)input.elementD;
  output[4] = (int)input.elementE;
  output[5] = (int)input.elementF;
  output[6] = (int)input.elementG;
+#if !defined(__ARM_ARCH)
  output[7] = (int)input.elementH;
+#endif
 };
 
 void test_function(int_pair input, global int* output) {
 // CHECK-NOT: spir_kernel
 // AMDGCN-NOT: define{{.*}} amdgpu_kernel void @test_function
 // CHECK: i64 %input.coerce0, i64 %input.coerce1, i32* nocapture %output
  output[0] = (int)input.a;
  output[1] = (int)input.b;
 }