[clang] Fallback to C calling convention for sycl_kernel attribute

[camc]

Resolves #161077

Fixes a crash. Let the device kernel calling convention be C when not compiling OpenCL. sycl_kernel in this context then has no effect, as expected.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: None (camc)

Changes

Resolves #161077

Fixes a crash. Let the device kernel calling convention be C when not compiling OpenCL. sycl_kernel in this context then has no effect, as expected.

---

Full diff: https://github.com/llvm/llvm-project/pull/161349.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/CodeGen/TargetInfo.cpp (+2-1)
• (added) clang/test/CodeGen/sycl-kernel.c (+5)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e8deae50e4cb0..74580b77353d5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -397,6 +397,7 @@ Bug Fixes to Attribute Support
 - Using ``[[gnu::cleanup(some_func)]]`` where some_func is annotated with
   ``[[gnu::error("some error")]]`` now correctly triggers an error. (#GH146520)
 - Fix a crash when the function name is empty in the `swift_name` attribute. (#GH157075)
+- Fix a crash when the ``sycl_kernel`` attribute is used while not compiling OpenCL (#GH161077).
 
 Bug Fixes to C++ Support
 ^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/CodeGen/TargetInfo.cpp b/clang/lib/CodeGen/TargetInfo.cpp
index 1e58c3f217812..4ff847f4f6ff6 100644
--- a/clang/lib/CodeGen/TargetInfo.cpp
+++ b/clang/lib/CodeGen/TargetInfo.cpp
@@ -123,8 +123,9 @@ unsigned TargetCodeGenInfo::getDeviceKernelCallingConv() const {
     // conventions; different targets might split structs passed as values
     // to multiple function arguments etc.
     return llvm::CallingConv::SPIR_KERNEL;
+  } else {
+    return llvm::CallingConv::C;
   }
-  llvm_unreachable("Unknown kernel calling convention");
 }
 
 void TargetCodeGenInfo::setOCLKernelStubCallingConvention(
diff --git a/clang/test/CodeGen/sycl-kernel.c b/clang/test/CodeGen/sycl-kernel.c
new file mode 100644
index 0000000000000..77410e5a216ff
--- /dev/null
+++ b/clang/test/CodeGen/sycl-kernel.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -emit-llvm %s -o /dev/null
+
+__attribute__((sycl_kernel)) void foo(int *ret) {
+  *ret = 1;
+}

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

[Fznamznon]

I'm not sure this is entirely right solution.
IMO we should probably ignore sycl_kernel attribute if the target is not offloading target, similar to amggpu_kernel attribute. However it maybe not worth doing if sycl_kernel attribute is going to be removed in favor of clang::sycl_kernel_entry_point attribute which BTW has the same crashing problem. cc @tahonermann

[sarnex]

I agree with Mariya on this one. we're going to run into a ton of problems if we are trying to use offloading attributes when not offloading. We should just ignore the attribute and throw the appropriate warning.

[camc]

Makes sense, I didn't see it was ignored correctly for amdgpu. It seems this is because isMultiSubjectAttrAllowedOnType returns false for non-amdgpu, so it is skipped and then not ignored and diagnosed in getCCForDeclaratorChunk. Not sure the purpose of this isMultiSubjectAttrAllowedOnType check, is it still necessary @sarnex? Doesn't seem ideal to have to split amdgpu/non-amdgpu cases.

[sarnex]

Right, the annoying problem is that the AMDGPU attribute is allowed both on types and functions, and all the others are allowed only on functions, so I had to allow the AMDGPU type case but bail on all others. As far as I know this logic is still required, but I don't think we can unify the logic unless we unify the allowed subjects for the attributes.

[camc]

Ah yeah that's annoying, I'll special case AMDGPU for now.

[sarnex]

Let me try removing it and see what explodes, will post my results here

[sarnex]

I got it down to one test failure, hopefully should have something tomorrow

[jhuber6]

The handling here seems absolutely inconsistent. Why is amdgpu_kernel considered a sycl kernel. Why doesn't device_kerel imply a kernel for AMDGPU but does for NVPTX? See https://godbolt.org/z/61f7bY3n6, you should be able to apply function attributes before the function declaration as far as I know.

[sarnex]

Yeah that's crazy. I guess I unified the attribute names but didn't completely unify the semantics. I don't know that we can do that for sure, but I'll try. To be honest I expect to hit a ton of nonsense.

[sarnex]

Actually there was only minimal nonsense, should have a PR shortly.