[CUDA][SPIRV] Assign global address space to CUDA kernel arguments

[shangwuyao]

Summary

This patch converts CUDA pointer kernel arguments with default address space to
CrossWorkGroup address space (__global in OpenCL). This is because Generic or
Function (OpenCL's private) is not supported as storage class for kernel pointer types.

[jlebar]

[CUDA][SPIRV] Convert CUDA kernels to SPIR-V kernels

Rephrase this? This patch is about kernel *arguments*, right?

[jlebar]

clang/lib/CodeGen/TargetInfo.cpp

10323

I am surprised by this change. Is the language mode HIP only when compiling for device? Or are you intentionally changing the behavior in HIP mode?

Same in SPIR.h

[shangwuyao]

clang/lib/CodeGen/TargetInfo.cpp

10323

We are targeting SPIRV so I think "compiling for device" is implied, I will let others comment on this to see if the assumption is correct. So this function can only be called when compiling for device, and won't be called when compiling for host.

Also tried compiling for device and host separately to see where exactly does the code diverge (to make sure those two functions are not called when compiling for host):

1. This classifyKernelArgumentType() function is called from here, which is only enabled when the calling convention is SPIR_KERNEL. And when compiling for host, the calling convention is C.

2. For the SPIR.h file, the TargetInfo::adjust function is called both when compiling for host and for device, see here, while the setAddressSpaceMap function is only called when compiling for device (SPIRV).

In conclusion, those two functions won't be reached when compiling for host.

[yaxunl]

clang/lib/CodeGen/TargetInfo.cpp

10323

LGTM.

[shangwuyao]

Thanks for the review, if it looks good, can we get this to land now? Otherwise more comments are welcome!

[jlebar]

In D119207#3327476, @shangwuyao wrote:

Thanks for the review, if it looks good, can we get this to land now? Otherwise more comments are welcome!

I'll land this for you!

At some point you should get commit access yourself, Shangwu.

[jlebar]

commit 9de4fc0f2d3b60542956f7e5254951d049edeb1f (HEAD -> main, origin/main, origin/HEAD)
Author: Shangwu Yao <[e-mail]>
Date:   Thu Feb 17 09:38:06 2022 -0800

    [CUDA][SPIRV] Assign global address space to CUDA kernel arguments

    This patch converts CUDA pointer kernel arguments with default address space to
    CrossWorkGroup address space (__global in OpenCL). This is because Generic or
    Function (OpenCL's private) is not supported as storage class for kernel pointer types.

    Differential Revision: https://reviews.llvm.org/D119207

[dyung]

Hi, the test you added is failing on the PS4 Linux bot, can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/17199

[shangwuyao]

In D119207#3330385, @dyung wrote:

Hi, the test you added is failing on the PS4 Linux bot, can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/17199

Looks like the compiled SPIR-V is slightly different for different build settings, for llvm-clang-x86_64-sie-ubuntu-fast, it is compiled to

define hidden spir_kernel void @_Z6kernelPi(i32 addrspace(1)* noundef %output.coerce) #0 {

so it is missing that extra hidden keyword.
And for clang-ve-ninja, it is compiled to

define spir_kernel void @_Z6kernelPi(i32 addrspace(1)* noundef %0) #0 {

so the kernel argument identifier is slightly different (%0 vs %output.coerce).

I could fix that, I wonder why it didn't trigger the same issue (for the hidden keyword) with this test tho, it is basically the same.

And why does those build test run only after merging? For future reference, can I try to run those myself before submitting?

For this change, should we do a rollback and then re-land it after applying the fix?

[dyung]

In D119207#3330494, @shangwuyao wrote:

In D119207#3330385, @dyung wrote:

Hi, the test you added is failing on the PS4 Linux bot, can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/17199

Looks like the compiled SPIR-V is slightly different for different build settings, for llvm-clang-x86_64-sie-ubuntu-fast, it is compiled to

define hidden spir_kernel void @_Z6kernelPi(i32 addrspace(1)* noundef %output.coerce) #0 {

so it is missing that extra hidden keyword.
And for clang-ve-ninja, it is compiled to

define spir_kernel void @_Z6kernelPi(i32 addrspace(1)* noundef %0) #0 {

so the kernel argument identifier is slightly different (%0 vs %output.coerce).

I could fix that, I wonder why it didn't trigger the same issue (for the hidden keyword) with this test tho, it is basically the same.

And why does those build test run only after merging? For future reference, can I try to run those myself before submitting?

These are build bots that run builds when new commits are detected. That's how they work. There is some pre-commit testing, but I'm not sure how that works exactly.

You can run these builds yourself before submitting, just extract the cmake command from the job and then run the build/test the normal way. If you need help reproducing a bot failure, an email to the bot owner can help you if you have trouble reproducing a failure.

For this change, should we do a rollback and then re-land it after applying the fix?

If you can fix it soon, a quick fix is fine. If you need time to investigate, a revert would be best until you can fix it so that the bot does not keep failing.

[ormris]

Hi Shangwu,

I've reverted this change to unblock the buildbots and our internal CI.

clang/lib/Basic/Targets/SPIR.h

void setAddressSpaceMap(bool DefaultIsGeneric) {
     AddrSpaceMap = DefaultIsGeneric ? &SPIRDefIsGenMap : &SPIRDefIsPrivMap;
   }
 
   void adjust(DiagnosticsEngine &Diags, LangOptions &Opts) override {
     TargetInfo::adjust(Diags, Opts);
     // FIXME: SYCL specification considers unannotated pointers and references
     // to be pointing to the generic address space. See section 5.9.3 of
     // SYCL 2020 specification.
-    // Currently, there is no way of representing SYCL's and HIP's default
+    // Currently, there is no way of representing SYCL's and HIP/CUDA's default
     // address space language semantic along with the semantics of embedded C's
     // default address space in the same address space map. Hence the map needs
     // to be reset to allow mapping to the desired value of 'Default' entry for
-    // SYCL and HIP.
+    // SYCL and HIP/CUDA.
     setAddressSpaceMap(
         /*DefaultIsGeneric=*/Opts.SYCLIsDevice ||
-        // The address mapping from HIP language for device code is only defined
-        // for SPIR-V.
-        (getTriple().isSPIRV() && Opts.HIP && Opts.CUDAIsDevice));
+        // The address mapping from HIP/CUDA language for device code is only
+        // defined for SPIR-V.
+        (getTriple().isSPIRV() && Opts.CUDAIsDevice));
   }
 
   void setSupportedOpenCLOpts() override {
     // Assume all OpenCL extensions and optional core features are supported
     // for SPIR and SPIR-V since they are generic targets.
     supportAllOpenCLOpts();
   }
 

clang/lib/CodeGen/TargetInfo.cpp

 } // End anonymous namespace.
 
 void CommonSPIRABIInfo::setCCs() {
   assert(getRuntimeCC() == llvm::CallingConv::C);
   RuntimeCC = llvm::CallingConv::SPIR_FUNC;
 }
 
 ABIArgInfo SPIRVABIInfo::classifyKernelArgumentType(QualType Ty) const {
-  if (getContext().getLangOpts().HIP) {
+  if (getContext().getLangOpts().CUDAIsDevice) {
     // Coerce pointer arguments with default address space to CrossWorkGroup
-    // pointers for HIPSPV. When the language mode is HIP, the SPIRTargetInfo
-    // maps cuda_device to SPIR-V's CrossWorkGroup address space.
+    // pointers for HIPSPV/CUDASPV. When the language mode is HIP/CUDA, the
+    // SPIRTargetInfo maps cuda_device to SPIR-V's CrossWorkGroup address space.
     llvm::Type *LTy = CGT.ConvertType(Ty);
     auto DefaultAS = getContext().getTargetAddressSpace(LangAS::Default);
     auto GlobalAS = getContext().getTargetAddressSpace(LangAS::cuda_device);
     auto *PtrTy = llvm::dyn_cast<llvm::PointerType>(LTy);
     if (PtrTy && PtrTy->getAddressSpace() == DefaultAS) {
       LTy = llvm::PointerType::getWithSamePointeeType(PtrTy, GlobalAS);
       return ABIArgInfo::getDirect(LTy, 0, nullptr, false);
     }

clang/test/CodeGenCUDASPIRV/kernel-argument.cu

+// Tests CUDA kernel arguments get global address space when targetting SPIR-V.
+
+// REQUIRES: clang-driver
+
+// RUN: %clang -emit-llvm --cuda-device-only --offload=spirv32 \
+// RUN:   -nocudalib -nocudainc %s -o %t.bc -c 2>&1
+// RUN: llvm-dis %t.bc -o %t.ll
+// RUN: FileCheck %s --input-file=%t.ll
+
+// RUN: %clang -emit-llvm --cuda-device-only --offload=spirv64 \
+// RUN:   -nocudalib -nocudainc %s -o %t.bc -c 2>&1
+// RUN: llvm-dis %t.bc -o %t.ll
+// RUN: FileCheck %s --input-file=%t.ll
+
+// CHECK: define spir_kernel void @_Z6kernelPi(i32 addrspace(1)* noundef %output.coerce)
+
+__attribute__((global)) void kernel(int* output) { *output = 1; }