[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki added a comment.

Thanks, @Anastasia.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf4d3cb4ca833: [HIPSPV] Add CUDA-SPIR-V address space 
mapping (authored by Anastasia).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

Files:
  clang/lib/Basic/Targets/SPIR.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp

Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(3)* @s to i32 addrspace(4)*
+  return 
+}
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -56,9 +56,14 @@
 0, // opencl_generic
 0, // opencl_global_device
 0, // opencl_global_host
-0, // cuda_device
-0, // cuda_constant
-0, // cuda_shared
+// cuda_* address space mapping is intended for HIPSPV (HIP to SPIR-V
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in
+// SPIR-V casts between constant and generic pointers are not allowed. For
+// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
+1, // cuda_constant
+3, // cuda_shared
 1, // sycl_global
 5, // sycl_global_device
 6, // sycl_global_host
@@ -74,6 +79,8 @@
 protected:
   BaseSPIRTargetInfo(const llvm::Triple , const TargetOptions &)
   : TargetInfo(Triple) {
+assert((Triple.isSPIR() || Triple.isSPIRV()) &&
+   "Invalid architecture for SPIR or SPIR-V.");
 assert(getTriple().getOS() == llvm::Triple::UnknownOS &&
"SPIR(-V) target must use unknown OS");
 assert(getTriple().getEnvironment() == llvm::Triple::UnknownEnvironment &&
@@ -137,11 +144,16 @@
 // 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 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.
-setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice);
+// Currently, there is no way of representing SYCL's and HIP'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.
+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));
   }
 
   void setSupportedOpenCLOpts() override {
@@ -159,6 +171,7 @@
 public:
   SPIRTargetInfo(const llvm::Triple , const TargetOptions )
   : BaseSPIRTargetInfo(Triple, Opts) {
+assert(Triple.isSPIR() && "Invalid architecture for SPIR.");
 assert(getTriple().getOS() == llvm::Triple::UnknownOS &&
"SPIR target must use unknown OS");
 assert(getTriple().getEnvironment() == llvm::Triple::UnknownEnvironment &&
@@ -177,6 +190,8 @@
 public:
   

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki added a comment.

@Anastasia, Could you please commit this patch to the LLVM for us? Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:233
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.

Anastasia wrote:
> linjamaki wrote:
> > Anastasia wrote:
> > > My guess is that this is not only HIP specific but for example the same 
> > > applies to SYCL.
> > > 
> > > I am not sure if it makes more sense to move this into a 
> > > `BaseSPIRTargetInfo` since this is not really SPIR-V specific logic. It 
> > > is just a clang design misalignment between two address space concepts 
> > > that has to be addressed properly at some point.
> > > 
> > The DefaultIsGeneric AS mapping is enabled for SYCL in the 
> > BaseSPIRTargetInfo::adjust (which also means the mapping is available for 
> > both the SPIR and SPIR-V targets). On the other hand, the AS mapping for 
> > HIPSPV is enabled in SPIRVTargetInfo::adjust only as we intend to emit 
> > SPIR-V only. I’m under the impression that this is what was wanted.
> I think the issues here is not related to the target but to the flaw in the 
> address space design in clang. So right now all languages that don't derive 
> the address space semantic from embedded C (SYCL/CUDA/HIP) would need to 
> reset the address space map. See FIXME comment in the definition of `adjust`.
> 
> So the right thing to do would be to set the address space map correctly 
> straight away based on the language being compiled for which would avoid 
> overriding this in `adjust`. But if we do override it then it makes more 
> sense to at least unify the logic among targets.
> 
> 
> > 
> > On the other hand, the AS mapping for HIPSPV is enabled in 
> > SPIRVTargetInfo::adjust only as we intend to emit SPIR-V only. 
> > 
> 
> 
> I am not really sure how you would support one target only.
> Clang architecture (at least originally) assumes that all languages can map 
> to all targets which in practice is not true in some cases. This is why we 
> need to provide an address space map even for targets that have no memory 
> segmented language compiled to it. 

> So the right thing to do would be to set the address space map correctly 
> straight away based on the language being compiled for which would avoid 
> overriding this in `adjust`. But if we do override it then it makes more 
> sense to at least unify the logic among targets.
> 

Since we are not sure how we would solve this issue optimally, we adjusted the 
patch to avoid adding more overrides for the `adjust` method and the logic 
previously in the `SPIRVTargetInfo::adjust` is moved to 
`BaseSPIRTargetInfo::adjust` with the SYCL.  Would this be sufficient for the 
functionality added by this patch?

> I am not really sure how you would support one target only.
> Clang architecture (at least originally) assumes that all languages can map 
> to all targets which in practice is not true in some cases. This is why we 
> need to provide an address space map even for targets that have no memory 
> segmented language compiled to it. 

“HIPSPV” is not meant to be a new language. We are just adjusting the address 
space mapping from the HIP language (for device code) to SPIR-V that suits 
better than the default mapping where all the HIP address spaces would be 
mapped to target address space zero. We map the address spaces to the suitable 
ones in the OpenCL standard, which works both for HIPCL (which uses the 
OpenCL-based runtime and the OpenCL SPIR-V profile) and HIPLZ (which uses the 
LZ-based runtime and also the OpenCL SPIR-V profile).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki updated this revision to Diff 387500.
linjamaki added a comment.

Rebase, add asserts and move address space map reset for HIP from 
SPIRVTargetInfo to BaseSPIRTargetInfo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

Files:
  clang/lib/Basic/Targets/SPIR.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp

Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(3)* @s to i32 addrspace(4)*
+  return 
+}
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -56,9 +56,14 @@
 0, // opencl_generic
 0, // opencl_global_device
 0, // opencl_global_host
-0, // cuda_device
-0, // cuda_constant
-0, // cuda_shared
+// cuda_* address space mapping is intended for HIPSPV (HIP to SPIR-V
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in
+// SPIR-V casts between constant and generic pointers are not allowed. For
+// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
+1, // cuda_constant
+3, // cuda_shared
 1, // sycl_global
 5, // sycl_global_device
 6, // sycl_global_host
@@ -74,6 +79,8 @@
 protected:
   BaseSPIRTargetInfo(const llvm::Triple , const TargetOptions &)
   : TargetInfo(Triple) {
+assert((Triple.isSPIR() || Triple.isSPIRV()) &&
+   "Invalid architecture for SPIR or SPIR-V.");
 assert(getTriple().getOS() == llvm::Triple::UnknownOS &&
"SPIR(-V) target must use unknown OS");
 assert(getTriple().getEnvironment() == llvm::Triple::UnknownEnvironment &&
@@ -137,11 +144,16 @@
 // 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 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.
-setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice);
+// Currently, there is no way of representing SYCL's and HIP'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.
+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));
   }
 
   void setSupportedOpenCLOpts() override {
@@ -159,6 +171,7 @@
 public:
   SPIRTargetInfo(const llvm::Triple , const TargetOptions )
   : BaseSPIRTargetInfo(Triple, Opts) {
+assert(Triple.isSPIR() && "Invalid architecture for SPIR.");
 assert(getTriple().getOS() == llvm::Triple::UnknownOS &&
"SPIR target must use unknown OS");
 assert(getTriple().getEnvironment() == llvm::Triple::UnknownEnvironment &&
@@ -177,6 +190,8 @@
 public:
   

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:233
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.

linjamaki wrote:
> Anastasia wrote:
> > My guess is that this is not only HIP specific but for example the same 
> > applies to SYCL.
> > 
> > I am not sure if it makes more sense to move this into a 
> > `BaseSPIRTargetInfo` since this is not really SPIR-V specific logic. It is 
> > just a clang design misalignment between two address space concepts that 
> > has to be addressed properly at some point.
> > 
> The DefaultIsGeneric AS mapping is enabled for SYCL in the 
> BaseSPIRTargetInfo::adjust (which also means the mapping is available for 
> both the SPIR and SPIR-V targets). On the other hand, the AS mapping for 
> HIPSPV is enabled in SPIRVTargetInfo::adjust only as we intend to emit SPIR-V 
> only. I’m under the impression that this is what was wanted.
I think the issues here is not related to the target but to the flaw in the 
address space design in clang. So right now all languages that don't derive the 
address space semantic from embedded C (SYCL/CUDA/HIP) would need to reset the 
address space map. See FIXME comment in the definition of `adjust`.

So the right thing to do would be to set the address space map correctly 
straight away based on the language being compiled for which would avoid 
overriding this in `adjust`. But if we do override it then it makes more sense 
to at least unify the logic among targets.


> 
> On the other hand, the AS mapping for HIPSPV is enabled in 
> SPIRVTargetInfo::adjust only as we intend to emit SPIR-V only. 
> 


I am not really sure how you would support one target only.
Clang architecture (at least originally) assumes that all languages can map to 
all targets which in practice is not true in some cases. This is why we need to 
provide an address space map even for targets that have no memory segmented 
language compiled to it. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki added a comment.

Gentle ping. Is anything needed to be addressed to get this patch accepted?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki updated this revision to Diff 381850.
linjamaki added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

Files:
  clang/lib/Basic/Targets/SPIR.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(3)* @s to i32 
addrspace(4)*
+  return 
+}
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -56,9 +56,14 @@
 0, // opencl_generic
 0, // opencl_global_device
 0, // opencl_global_host
-0, // cuda_device
-0, // cuda_constant
-0, // cuda_shared
+// cuda_* address space mapping is intended for HIPSPV (HIP to SPIR-V
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in
+// SPIR-V casts between constant and generic pointers are not allowed. For
+// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
+1, // cuda_constant
+3, // cuda_shared
 1, // sycl_global
 5, // sycl_global_device
 6, // sycl_global_host
@@ -219,6 +224,16 @@
   bool hasFeature(StringRef Feature) const override {
 return Feature == "spirv";
   }
+
+  void adjust(DiagnosticsEngine , LangOptions ) override {
+BaseSPIRTargetInfo::adjust(Diags, Opts);
+// Guarded so we don't override address space map setting set by
+// BaseSPIRTargetInfo::adjust.
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.
+  setAddressSpaceMap(/*DefaultIsGeneric=*/true);
+  }
 };
 
 class LLVM_LIBRARY_VISIBILITY SPIRV32TargetInfo : public SPIRVTargetInfo {


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

bader wrote:
> Anastasia wrote:
> > bader wrote:
> > > Anastasia wrote:
> > > > bader wrote:
> > > > > Anastasia wrote:
> > > > > > linjamaki wrote:
> > > > > > > bader wrote:
> > > > > > > > keryell wrote:
> > > > > > > > > Anastasia wrote:
> > > > > > > > > > bader wrote:
> > > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > > I am slightly confused as in the LLVM project those 
> > > > > > > > > > > > address spaces are for SPIR not SPIR-V though. It is 
> > > > > > > > > > > > however used outside of LLVM project by some tools like 
> > > > > > > > > > > > SPIRV-LLVM Translator as a path to SPIR-V, but it has 
> > > > > > > > > > > > only been done as a workaround since we had no SPIR-V 
> > > > > > > > > > > > support in the LLVM project yet. And if we are adding 
> > > > > > > > > > > > it let's do it clean to avoid/resolve any confusion.
> > > > > > > > > > > > 
> > > > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > > > > 
> > > > > > > > > > > > So if you are interested in SPIR-V and not SPIR you 
> > > > > > > > > > > > should probably add a new target that will make things 
> > > > > > > > > > > > cleaner.
> > > > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > > > 
> > > > > > > > > > > @Anastasia, could you elaborate more on the difference 
> > > > > > > > > > > between SPIR and SPIR-V?
> > > > > > > > > > > I would like to understand what these terms mean in the 
> > > > > > > > > > > context of LLVM project.
> > > > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > > > different intermediate formats.
> > > > > > > > > > 
> > > > > > > > > > The important thing to highlight is that it is not 
> > > > > > > > > > impossible that some vendors use SPIR (without using 
> > > > > > > > > > SPIR-V) even despite the fact it has been discontinued by 
> > > > > > > > > > Khronos. 
> > > > > > > > > > 
> > > > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM 
> > > > > > > > > > project yet.
> > > > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > > > different intermediate formats.
> > > > > > > > > > 
> > > > > > > > > > The important thing to highlight is that it is not 
> > > > > > > > > > impossible that some vendors use SPIR (without using 
> > > > > > > > > > SPIR-V) even despite the fact it has been discontinued by 
> > > > > > > > > > Khronos. 
> > > > > > > > > > 
> > > > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM 
> > > > > > > > > > project yet.
> > > > > > > > > 
> > > > > > > > > All the official Xilinx OpenCL stack is based on legacy SPIR 
> > > > > > > > > (encoded in LLVM 6.x IR but this is another story) and I 
> > > > > > > > > suspect this is the case for other companies.
> > > > > > > > > So, do not deprecate or discontinue, please. :-)
> > > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > > that some vendors use SPIR (without using SPIR-V) even 
> > > > > > > > > despite the fact it has been discontinued by Khronos.
> > > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM 
> > > > > > > > > project yet.
> > > > > > > > 
> > > > > > > > Strictly speaking `SPIR` is not defined as an intermediate 
> > > > > > > > language. Khronos defines `SPIR-1.2` and `SPIR-2.0` formats 
> > > > > > > > which are based on LLVM 3.2 and LLVM 3.4 version 
> > > > > > > > (https://www.khronos.org/spir/). There is no definition of SPIR 
> > > > > > > > format based on current version of LLVM IR. Another note is 
> > > > > > > > that metadata and intrinsics emitted for OpenCL with clang-14 
> > > > > > > > doesn't follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > > > > > > 
> > > > > > > > I always think of LLVM IR as leaving thing that is subject to 
> > > > > > > > change by LLVM community, so tools working with LLVM IR must 
> > > > > > > > adjust to the particular version (e.g. release version like 
> > > > > > > > LLVM 13 or ToT). We apply this logic to SPIRV-LLVM-Translator 
> > > > > > > > tool and update it according to LLVM format changes (e.g. 
> > > > > > > > kernel argument information defined in Khronos spec must be 
> > > > > > > > named metadata whereas clang emits function metadata).
> > > > > > > > 
> > > > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > > > spaces are for SPIR not SPIR-V though.
> > > > > > > > [skip]
> > > > > > > > > Their conceptual differences are just that they are two 
> > > > 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > linjamaki wrote:
> > > > > > bader wrote:
> > > > > > > keryell wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > bader wrote:
> > > > > > > > > > Anastasia wrote:
> > > > > > > > > > > I am slightly confused as in the LLVM project those 
> > > > > > > > > > > address spaces are for SPIR not SPIR-V though. It is 
> > > > > > > > > > > however used outside of LLVM project by some tools like 
> > > > > > > > > > > SPIRV-LLVM Translator as a path to SPIR-V, but it has 
> > > > > > > > > > > only been done as a workaround since we had no SPIR-V 
> > > > > > > > > > > support in the LLVM project yet. And if we are adding it 
> > > > > > > > > > > let's do it clean to avoid/resolve any confusion.
> > > > > > > > > > > 
> > > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > > > 
> > > > > > > > > > > So if you are interested in SPIR-V and not SPIR you 
> > > > > > > > > > > should probably add a new target that will make things 
> > > > > > > > > > > cleaner.
> > > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > > 
> > > > > > > > > > @Anastasia, could you elaborate more on the difference 
> > > > > > > > > > between SPIR and SPIR-V?
> > > > > > > > > > I would like to understand what these terms mean in the 
> > > > > > > > > > context of LLVM project.
> > > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > > different intermediate formats.
> > > > > > > > > 
> > > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > > that some vendors use SPIR (without using SPIR-V) even 
> > > > > > > > > despite the fact it has been discontinued by Khronos. 
> > > > > > > > > 
> > > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM 
> > > > > > > > > project yet.
> > > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > > different intermediate formats.
> > > > > > > > > 
> > > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > > that some vendors use SPIR (without using SPIR-V) even 
> > > > > > > > > despite the fact it has been discontinued by Khronos. 
> > > > > > > > > 
> > > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM 
> > > > > > > > > project yet.
> > > > > > > > 
> > > > > > > > All the official Xilinx OpenCL stack is based on legacy SPIR 
> > > > > > > > (encoded in LLVM 6.x IR but this is another story) and I 
> > > > > > > > suspect this is the case for other companies.
> > > > > > > > So, do not deprecate or discontinue, please. :-)
> > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > > the fact it has been discontinued by Khronos.
> > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > > yet.
> > > > > > > 
> > > > > > > Strictly speaking `SPIR` is not defined as an intermediate 
> > > > > > > language. Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which 
> > > > > > > are based on LLVM 3.2 and LLVM 3.4 version 
> > > > > > > (https://www.khronos.org/spir/). There is no definition of SPIR 
> > > > > > > format based on current version of LLVM IR. Another note is that 
> > > > > > > metadata and intrinsics emitted for OpenCL with clang-14 doesn't 
> > > > > > > follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > > > > > 
> > > > > > > I always think of LLVM IR as leaving thing that is subject to 
> > > > > > > change by LLVM community, so tools working with LLVM IR must 
> > > > > > > adjust to the particular version (e.g. release version like LLVM 
> > > > > > > 13 or ToT). We apply this logic to SPIRV-LLVM-Translator tool and 
> > > > > > > update it according to LLVM format changes (e.g. kernel argument 
> > > > > > > information defined in Khronos spec must be named metadata 
> > > > > > > whereas clang emits function metadata).
> > > > > > > 
> > > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > > spaces are for SPIR not SPIR-V though.
> > > > > > > [skip]
> > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > different intermediate formats.
> > > > > > > 
> > > > > > > If this is the only difference, I don't think it a good idea to 
> > > > > > > create another LLVM target to separate SPIR and SPIR-V. From my 
> > > > 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

bader wrote:
> Anastasia wrote:
> > bader wrote:
> > > Anastasia wrote:
> > > > linjamaki wrote:
> > > > > bader wrote:
> > > > > > keryell wrote:
> > > > > > > Anastasia wrote:
> > > > > > > > bader wrote:
> > > > > > > > > Anastasia wrote:
> > > > > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > > > > spaces are for SPIR not SPIR-V though. It is however used 
> > > > > > > > > > outside of LLVM project by some tools like SPIRV-LLVM 
> > > > > > > > > > Translator as a path to SPIR-V, but it has only been done 
> > > > > > > > > > as a workaround since we had no SPIR-V support in the LLVM 
> > > > > > > > > > project yet. And if we are adding it let's do it clean to 
> > > > > > > > > > avoid/resolve any confusion.
> > > > > > > > > > 
> > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > > 
> > > > > > > > > > So if you are interested in SPIR-V and not SPIR you should 
> > > > > > > > > > probably add a new target that will make things cleaner.
> > > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > 
> > > > > > > > > @Anastasia, could you elaborate more on the difference 
> > > > > > > > > between SPIR and SPIR-V?
> > > > > > > > > I would like to understand what these terms mean in the 
> > > > > > > > > context of LLVM project.
> > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > different intermediate formats.
> > > > > > > > 
> > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > > the fact it has been discontinued by Khronos. 
> > > > > > > > 
> > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > > yet.
> > > > > > > > Their conceptual differences are just that they are two 
> > > > > > > > different intermediate formats.
> > > > > > > > 
> > > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > > the fact it has been discontinued by Khronos. 
> > > > > > > > 
> > > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > > yet.
> > > > > > > 
> > > > > > > All the official Xilinx OpenCL stack is based on legacy SPIR 
> > > > > > > (encoded in LLVM 6.x IR but this is another story) and I suspect 
> > > > > > > this is the case for other companies.
> > > > > > > So, do not deprecate or discontinue, please. :-)
> > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > the fact it has been discontinued by Khronos.
> > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > yet.
> > > > > > 
> > > > > > Strictly speaking `SPIR` is not defined as an intermediate 
> > > > > > language. Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which 
> > > > > > are based on LLVM 3.2 and LLVM 3.4 version 
> > > > > > (https://www.khronos.org/spir/). There is no definition of SPIR 
> > > > > > format based on current version of LLVM IR. Another note is that 
> > > > > > metadata and intrinsics emitted for OpenCL with clang-14 doesn't 
> > > > > > follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > > > > 
> > > > > > I always think of LLVM IR as leaving thing that is subject to 
> > > > > > change by LLVM community, so tools working with LLVM IR must adjust 
> > > > > > to the particular version (e.g. release version like LLVM 13 or 
> > > > > > ToT). We apply this logic to SPIRV-LLVM-Translator tool and update 
> > > > > > it according to LLVM format changes (e.g. kernel argument 
> > > > > > information defined in Khronos spec must be named metadata whereas 
> > > > > > clang emits function metadata).
> > > > > > 
> > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > spaces are for SPIR not SPIR-V though.
> > > > > > [skip]
> > > > > > > Their conceptual differences are just that they are two different 
> > > > > > > intermediate formats.
> > > > > > 
> > > > > > If this is the only difference, I don't think it a good idea to 
> > > > > > create another LLVM target to separate SPIR and SPIR-V. From my 
> > > > > > point of view it creates logic ambiguity and code duplication with 
> > > > > > no additional value. @Anastasia, what problems do you see if we 
> > > > > > continue treating LLVM IR with spir* 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > linjamaki wrote:
> > > > bader wrote:
> > > > > keryell wrote:
> > > > > > Anastasia wrote:
> > > > > > > bader wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > > > spaces are for SPIR not SPIR-V though. It is however used 
> > > > > > > > > outside of LLVM project by some tools like SPIRV-LLVM 
> > > > > > > > > Translator as a path to SPIR-V, but it has only been done as 
> > > > > > > > > a workaround since we had no SPIR-V support in the LLVM 
> > > > > > > > > project yet. And if we are adding it let's do it clean to 
> > > > > > > > > avoid/resolve any confusion.
> > > > > > > > > 
> > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > > 
> > > > > > > > > So if you are interested in SPIR-V and not SPIR you should 
> > > > > > > > > probably add a new target that will make things cleaner.
> > > > > > > > > I think we need to keep both because some vendors do 
> > > > > > > > > target/use SPIR but not SPIR-V.
> > > > > > > > 
> > > > > > > > @Anastasia, could you elaborate more on the difference between 
> > > > > > > > SPIR and SPIR-V?
> > > > > > > > I would like to understand what these terms mean in the context 
> > > > > > > > of LLVM project.
> > > > > > > Their conceptual differences are just that they are two different 
> > > > > > > intermediate formats.
> > > > > > > 
> > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > the fact it has been discontinued by Khronos. 
> > > > > > > 
> > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > yet.
> > > > > > > Their conceptual differences are just that they are two different 
> > > > > > > intermediate formats.
> > > > > > > 
> > > > > > > The important thing to highlight is that it is not impossible 
> > > > > > > that some vendors use SPIR (without using SPIR-V) even despite 
> > > > > > > the fact it has been discontinued by Khronos. 
> > > > > > > 
> > > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project 
> > > > > > > yet.
> > > > > > 
> > > > > > All the official Xilinx OpenCL stack is based on legacy SPIR 
> > > > > > (encoded in LLVM 6.x IR but this is another story) and I suspect 
> > > > > > this is the case for other companies.
> > > > > > So, do not deprecate or discontinue, please. :-)
> > > > > > The important thing to highlight is that it is not impossible that 
> > > > > > some vendors use SPIR (without using SPIR-V) even despite the fact 
> > > > > > it has been discontinued by Khronos.
> > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > > 
> > > > > Strictly speaking `SPIR` is not defined as an intermediate language. 
> > > > > Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which are based on 
> > > > > LLVM 3.2 and LLVM 3.4 version (https://www.khronos.org/spir/). There 
> > > > > is no definition of SPIR format based on current version of LLVM IR. 
> > > > > Another note is that metadata and intrinsics emitted for OpenCL with 
> > > > > clang-14 doesn't follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > > > 
> > > > > I always think of LLVM IR as leaving thing that is subject to change 
> > > > > by LLVM community, so tools working with LLVM IR must adjust to the 
> > > > > particular version (e.g. release version like LLVM 13 or ToT). We 
> > > > > apply this logic to SPIRV-LLVM-Translator tool and update it 
> > > > > according to LLVM format changes (e.g. kernel argument information 
> > > > > defined in Khronos spec must be named metadata whereas clang emits 
> > > > > function metadata).
> > > > > 
> > > > > > I am slightly confused as in the LLVM project those address spaces 
> > > > > > are for SPIR not SPIR-V though.
> > > > > [skip]
> > > > > > Their conceptual differences are just that they are two different 
> > > > > > intermediate formats.
> > > > > 
> > > > > If this is the only difference, I don't think it a good idea to 
> > > > > create another LLVM target to separate SPIR and SPIR-V. From my point 
> > > > > of view it creates logic ambiguity and code duplication with no 
> > > > > additional value. @Anastasia, what problems do you see if we continue 
> > > > > treating LLVM IR with spir* target triple as LLVM IR representation 
> > > > > of SPIR-V format?
> > > > The state of SPIR 1.2/2.0 in Clang seems to be that the SPIR target has 
> > > > transformed to mean “SPIR 1.2/2.0 derivative”, but that 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki updated this revision to Diff 375221.
linjamaki added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

Files:
  clang/lib/Basic/Targets/SPIR.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(3)* @s to i32 
addrspace(4)*
+  return 
+}
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -56,9 +56,14 @@
 0, // opencl_generic
 0, // opencl_global_device
 0, // opencl_global_host
-0, // cuda_device
-0, // cuda_constant
-0, // cuda_shared
+// cuda_* address space mapping is intended for HIPSPV (HIP to SPIR-V
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in
+// SPIR-V casts between constant and generic pointers are not allowed. For
+// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
+1, // cuda_constant
+3, // cuda_shared
 1, // sycl_global
 5, // sycl_global_device
 6, // sycl_global_host
@@ -219,6 +224,16 @@
   bool hasFeature(StringRef Feature) const override {
 return Feature == "spirv";
   }
+
+  void adjust(DiagnosticsEngine , LangOptions ) override {
+BaseSPIRTargetInfo::adjust(Diags, Opts);
+// Guarded so we don't override address space map setting set by
+// BaseSPIRTargetInfo::adjust.
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.
+  setAddressSpaceMap(/*DefaultIsGeneric=*/true);
+  }
 };
 
 class LLVM_LIBRARY_VISIBILITY SPIRV32TargetInfo : public SPIRVTargetInfo {


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki updated this revision to Diff 375219.
linjamaki added a comment.
Herald added subscribers: llvm-commits, dexonsmith, hiraditya.
Herald added a project: LLVM.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/Basic/Targets.cpp
  clang/lib/Basic/Targets/SPIR.cpp
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Headers/opencl-c-base.h
  clang/lib/Headers/opencl-c.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
  clang/test/CodeGenOpenCL/spirv_target.cl
  clang/test/Headers/opencl-c-header.cl
  clang/test/Preprocessor/predefined-macros.c
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp
  llvm/unittests/ADT/TripleTest.cpp

Index: llvm/unittests/ADT/TripleTest.cpp
===
--- llvm/unittests/ADT/TripleTest.cpp
+++ llvm/unittests/ADT/TripleTest.cpp
@@ -224,6 +224,16 @@
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
   EXPECT_EQ(Triple::UnknownOS, T.getOS());
 
+  T = Triple("spirv32-unknown-unknown");
+  EXPECT_EQ(Triple::spirv32, T.getArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
+  T = Triple("spirv64-unknown-unknown");
+  EXPECT_EQ(Triple::spirv64, T.getArch());
+  EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
+  EXPECT_EQ(Triple::UnknownOS, T.getOS());
+
   T = Triple("x86_64-unknown-ananas");
   EXPECT_EQ(Triple::x86_64, T.getArch());
   EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
@@ -865,6 +875,16 @@
   EXPECT_FALSE(T.isArch32Bit());
   EXPECT_TRUE(T.isArch64Bit());
 
+  T.setArch(Triple::spirv32);
+  EXPECT_FALSE(T.isArch16Bit());
+  EXPECT_TRUE(T.isArch32Bit());
+  EXPECT_FALSE(T.isArch64Bit());
+
+  T.setArch(Triple::spirv64);
+  EXPECT_FALSE(T.isArch16Bit());
+  EXPECT_FALSE(T.isArch32Bit());
+  EXPECT_TRUE(T.isArch64Bit());
+
   T.setArch(Triple::sparc);
   EXPECT_FALSE(T.isArch16Bit());
   EXPECT_TRUE(T.isArch32Bit());
@@ -1000,6 +1020,14 @@
   EXPECT_EQ(Triple::spir, T.get32BitArchVariant().getArch());
   EXPECT_EQ(Triple::spir64, T.get64BitArchVariant().getArch());
 
+  T.setArch(Triple::spirv32);
+  EXPECT_EQ(Triple::spirv32, T.get32BitArchVariant().getArch());
+  EXPECT_EQ(Triple::spirv64, T.get64BitArchVariant().getArch());
+
+  T.setArch(Triple::spirv64);
+  EXPECT_EQ(Triple::spirv32, T.get32BitArchVariant().getArch());
+  EXPECT_EQ(Triple::spirv64, T.get64BitArchVariant().getArch());
+
   T.setArch(Triple::wasm32);
   EXPECT_EQ(Triple::wasm32, T.get32BitArchVariant().getArch());
   EXPECT_EQ(Triple::wasm64, T.get64BitArchVariant().getArch());
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -67,6 +67,8 @@
   case sparcv9:return "sparcv9";
   case spir64: return "spir64";
   case spir:   return "spir";
+  case spirv32:return "spirv32";
+  case spirv64:return "spirv64";
   case systemz:return "s390x";
   case tce:return "tce";
   case tcele:  return "tcele";
@@ -147,6 +149,10 @@
 
   case spir:
   case spir64:  return "spir";
+
+  case spirv32:
+  case spirv64: return "spirv";
+
   case kalimba: return "kalimba";
   case lanai:   return "lanai";
   case shave:   return "shave";
@@ -323,6 +329,8 @@
 .Case("hsail64", hsail64)
 .Case("spir", spir)
 .Case("spir64", spir64)
+.Case("spirv32", spirv32)
+.Case("spirv64", spirv64)
 .Case("kalimba", kalimba)
 .Case("lanai", lanai)
 .Case("shave", shave)
@@ -456,6 +464,8 @@
 .Case("hsail64", Triple::hsail64)
 .Case("spir", Triple::spir)
 .Case("spir64", Triple::spir64)
+.Case("spirv32", Triple::spirv32)
+.Case("spirv64", Triple::spirv64)
 .StartsWith("kalimba", Triple::kalimba)
 .Case("lanai", Triple::lanai)
 .Case("renderscript32", Triple::renderscript32)
@@ -753,6 +763,11 @@
   case Triple::wasm32:
   case Triple::wasm64:
 return Triple::Wasm;
+
+  case Triple::spirv32:
+  case Triple::spirv64:
+// TODO: In future this will be Triple::SPIRV.
+return Triple::UnknownObjectFormat;
   }
   llvm_unreachable("unknown architecture");
 }
@@ -1298,6 +1313,7 @@
   case llvm::Triple::sparc:
   case llvm::Triple::sparcel:
   case llvm::Triple::spir:
+  case llvm::Triple::spirv32:
   case llvm::Triple::tce:
   case llvm::Triple::tcele:
   case llvm::Triple::thumb:
@@ -1324,6 +1340,7 @@
   case llvm::Triple::riscv64:
   case llvm::Triple::sparcv9:
   case llvm::Triple::spir64:
+  case llvm::Triple::spirv64:
   case llvm::Triple::systemz:
   case llvm::Triple::ve:
   case llvm::Triple::wasm64:
@@ -1383,6 +1400,7 @@
   case Triple::sparc:
   case 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

bader wrote:
> Anastasia wrote:
> > linjamaki wrote:
> > > bader wrote:
> > > > keryell wrote:
> > > > > Anastasia wrote:
> > > > > > bader wrote:
> > > > > > > Anastasia wrote:
> > > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > > spaces are for SPIR not SPIR-V though. It is however used 
> > > > > > > > outside of LLVM project by some tools like SPIRV-LLVM 
> > > > > > > > Translator as a path to SPIR-V, but it has only been done as a 
> > > > > > > > workaround since we had no SPIR-V support in the LLVM project 
> > > > > > > > yet. And if we are adding it let's do it clean to avoid/resolve 
> > > > > > > > any confusion.
> > > > > > > > 
> > > > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > > > SPIR but not SPIR-V.
> > > > > > > > 
> > > > > > > > So if you are interested in SPIR-V and not SPIR you should 
> > > > > > > > probably add a new target that will make things cleaner.
> > > > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > > > SPIR but not SPIR-V.
> > > > > > > 
> > > > > > > @Anastasia, could you elaborate more on the difference between 
> > > > > > > SPIR and SPIR-V?
> > > > > > > I would like to understand what these terms mean in the context 
> > > > > > > of LLVM project.
> > > > > > Their conceptual differences are just that they are two different 
> > > > > > intermediate formats.
> > > > > > 
> > > > > > The important thing to highlight is that it is not impossible that 
> > > > > > some vendors use SPIR (without using SPIR-V) even despite the fact 
> > > > > > it has been discontinued by Khronos. 
> > > > > > 
> > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > > > Their conceptual differences are just that they are two different 
> > > > > > intermediate formats.
> > > > > > 
> > > > > > The important thing to highlight is that it is not impossible that 
> > > > > > some vendors use SPIR (without using SPIR-V) even despite the fact 
> > > > > > it has been discontinued by Khronos. 
> > > > > > 
> > > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > > 
> > > > > All the official Xilinx OpenCL stack is based on legacy SPIR (encoded 
> > > > > in LLVM 6.x IR but this is another story) and I suspect this is the 
> > > > > case for other companies.
> > > > > So, do not deprecate or discontinue, please. :-)
> > > > > The important thing to highlight is that it is not impossible that 
> > > > > some vendors use SPIR (without using SPIR-V) even despite the fact it 
> > > > > has been discontinued by Khronos.
> > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > 
> > > > Strictly speaking `SPIR` is not defined as an intermediate language. 
> > > > Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which are based on 
> > > > LLVM 3.2 and LLVM 3.4 version (https://www.khronos.org/spir/). There is 
> > > > no definition of SPIR format based on current version of LLVM IR. 
> > > > Another note is that metadata and intrinsics emitted for OpenCL with 
> > > > clang-14 doesn't follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > > 
> > > > I always think of LLVM IR as leaving thing that is subject to change by 
> > > > LLVM community, so tools working with LLVM IR must adjust to the 
> > > > particular version (e.g. release version like LLVM 13 or ToT). We apply 
> > > > this logic to SPIRV-LLVM-Translator tool and update it according to 
> > > > LLVM format changes (e.g. kernel argument information defined in 
> > > > Khronos spec must be named metadata whereas clang emits function 
> > > > metadata).
> > > > 
> > > > > I am slightly confused as in the LLVM project those address spaces 
> > > > > are for SPIR not SPIR-V though.
> > > > [skip]
> > > > > Their conceptual differences are just that they are two different 
> > > > > intermediate formats.
> > > > 
> > > > If this is the only difference, I don't think it a good idea to create 
> > > > another LLVM target to separate SPIR and SPIR-V. From my point of view 
> > > > it creates logic ambiguity and code duplication with no additional 
> > > > value. @Anastasia, what problems do you see if we continue treating 
> > > > LLVM IR with spir* target triple as LLVM IR representation of SPIR-V 
> > > > format?
> > > The state of SPIR 1.2/2.0 in Clang seems to be that the SPIR target has 
> > > transformed to mean “SPIR 1.2/2.0 derivative”, but that does not still 
> > > make it SPIR-V, which is not based on LLVM IR. When one is targeting 
> > > spir* there is ambiguity on whether one is aiming to produce the 
> > > old-SPIR-derivative or SPIR-V. 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> linjamaki wrote:
> > bader wrote:
> > > keryell wrote:
> > > > Anastasia wrote:
> > > > > bader wrote:
> > > > > > Anastasia wrote:
> > > > > > > I am slightly confused as in the LLVM project those address 
> > > > > > > spaces are for SPIR not SPIR-V though. It is however used outside 
> > > > > > > of LLVM project by some tools like SPIRV-LLVM Translator as a 
> > > > > > > path to SPIR-V, but it has only been done as a workaround since 
> > > > > > > we had no SPIR-V support in the LLVM project yet. And if we are 
> > > > > > > adding it let's do it clean to avoid/resolve any confusion.
> > > > > > > 
> > > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > > SPIR but not SPIR-V.
> > > > > > > 
> > > > > > > So if you are interested in SPIR-V and not SPIR you should 
> > > > > > > probably add a new target that will make things cleaner.
> > > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > > SPIR but not SPIR-V.
> > > > > > 
> > > > > > @Anastasia, could you elaborate more on the difference between SPIR 
> > > > > > and SPIR-V?
> > > > > > I would like to understand what these terms mean in the context of 
> > > > > > LLVM project.
> > > > > Their conceptual differences are just that they are two different 
> > > > > intermediate formats.
> > > > > 
> > > > > The important thing to highlight is that it is not impossible that 
> > > > > some vendors use SPIR (without using SPIR-V) even despite the fact it 
> > > > > has been discontinued by Khronos. 
> > > > > 
> > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > > Their conceptual differences are just that they are two different 
> > > > > intermediate formats.
> > > > > 
> > > > > The important thing to highlight is that it is not impossible that 
> > > > > some vendors use SPIR (without using SPIR-V) even despite the fact it 
> > > > > has been discontinued by Khronos. 
> > > > > 
> > > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > 
> > > > All the official Xilinx OpenCL stack is based on legacy SPIR (encoded 
> > > > in LLVM 6.x IR but this is another story) and I suspect this is the 
> > > > case for other companies.
> > > > So, do not deprecate or discontinue, please. :-)
> > > > The important thing to highlight is that it is not impossible that some 
> > > > vendors use SPIR (without using SPIR-V) even despite the fact it has 
> > > > been discontinued by Khronos.
> > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > 
> > > Strictly speaking `SPIR` is not defined as an intermediate language. 
> > > Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which are based on LLVM 
> > > 3.2 and LLVM 3.4 version (https://www.khronos.org/spir/). There is no 
> > > definition of SPIR format based on current version of LLVM IR. Another 
> > > note is that metadata and intrinsics emitted for OpenCL with clang-14 
> > > doesn't follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > > 
> > > I always think of LLVM IR as leaving thing that is subject to change by 
> > > LLVM community, so tools working with LLVM IR must adjust to the 
> > > particular version (e.g. release version like LLVM 13 or ToT). We apply 
> > > this logic to SPIRV-LLVM-Translator tool and update it according to LLVM 
> > > format changes (e.g. kernel argument information defined in Khronos spec 
> > > must be named metadata whereas clang emits function metadata).
> > > 
> > > > I am slightly confused as in the LLVM project those address spaces are 
> > > > for SPIR not SPIR-V though.
> > > [skip]
> > > > Their conceptual differences are just that they are two different 
> > > > intermediate formats.
> > > 
> > > If this is the only difference, I don't think it a good idea to create 
> > > another LLVM target to separate SPIR and SPIR-V. From my point of view it 
> > > creates logic ambiguity and code duplication with no additional value. 
> > > @Anastasia, what problems do you see if we continue treating LLVM IR with 
> > > spir* target triple as LLVM IR representation of SPIR-V format?
> > The state of SPIR 1.2/2.0 in Clang seems to be that the SPIR target has 
> > transformed to mean “SPIR 1.2/2.0 derivative”, but that does not still make 
> > it SPIR-V, which is not based on LLVM IR. When one is targeting spir* there 
> > is ambiguity on whether one is aiming to produce the old-SPIR-derivative or 
> > SPIR-V. Considering that there are still SPIR-derivative consumers, in my 
> > opinion we should have separate LLVM targets for SPIR-V to have explicit 
> > disambiguation of intent for producing the SPIR-derivative vs SPIR-V.
> 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

linjamaki wrote:
> bader wrote:
> > keryell wrote:
> > > Anastasia wrote:
> > > > bader wrote:
> > > > > Anastasia wrote:
> > > > > > I am slightly confused as in the LLVM project those address spaces 
> > > > > > are for SPIR not SPIR-V though. It is however used outside of LLVM 
> > > > > > project by some tools like SPIRV-LLVM Translator as a path to 
> > > > > > SPIR-V, but it has only been done as a workaround since we had no 
> > > > > > SPIR-V support in the LLVM project yet. And if we are adding it 
> > > > > > let's do it clean to avoid/resolve any confusion.
> > > > > > 
> > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > SPIR but not SPIR-V.
> > > > > > 
> > > > > > So if you are interested in SPIR-V and not SPIR you should probably 
> > > > > > add a new target that will make things cleaner.
> > > > > > I think we need to keep both because some vendors do target/use 
> > > > > > SPIR but not SPIR-V.
> > > > > 
> > > > > @Anastasia, could you elaborate more on the difference between SPIR 
> > > > > and SPIR-V?
> > > > > I would like to understand what these terms mean in the context of 
> > > > > LLVM project.
> > > > Their conceptual differences are just that they are two different 
> > > > intermediate formats.
> > > > 
> > > > The important thing to highlight is that it is not impossible that some 
> > > > vendors use SPIR (without using SPIR-V) even despite the fact it has 
> > > > been discontinued by Khronos. 
> > > > 
> > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > > Their conceptual differences are just that they are two different 
> > > > intermediate formats.
> > > > 
> > > > The important thing to highlight is that it is not impossible that some 
> > > > vendors use SPIR (without using SPIR-V) even despite the fact it has 
> > > > been discontinued by Khronos. 
> > > > 
> > > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > 
> > > All the official Xilinx OpenCL stack is based on legacy SPIR (encoded in 
> > > LLVM 6.x IR but this is another story) and I suspect this is the case for 
> > > other companies.
> > > So, do not deprecate or discontinue, please. :-)
> > > The important thing to highlight is that it is not impossible that some 
> > > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > > discontinued by Khronos.
> > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > 
> > Strictly speaking `SPIR` is not defined as an intermediate language. 
> > Khronos defines `SPIR-1.2` and `SPIR-2.0` formats which are based on LLVM 
> > 3.2 and LLVM 3.4 version (https://www.khronos.org/spir/). There is no 
> > definition of SPIR format based on current version of LLVM IR. Another note 
> > is that metadata and intrinsics emitted for OpenCL with clang-14 doesn't 
> > follow neither `SPIR-1.2` nor `SPIR-2.0`.
> > 
> > I always think of LLVM IR as leaving thing that is subject to change by 
> > LLVM community, so tools working with LLVM IR must adjust to the particular 
> > version (e.g. release version like LLVM 13 or ToT). We apply this logic to 
> > SPIRV-LLVM-Translator tool and update it according to LLVM format changes 
> > (e.g. kernel argument information defined in Khronos spec must be named 
> > metadata whereas clang emits function metadata).
> > 
> > > I am slightly confused as in the LLVM project those address spaces are 
> > > for SPIR not SPIR-V though.
> > [skip]
> > > Their conceptual differences are just that they are two different 
> > > intermediate formats.
> > 
> > If this is the only difference, I don't think it a good idea to create 
> > another LLVM target to separate SPIR and SPIR-V. From my point of view it 
> > creates logic ambiguity and code duplication with no additional value. 
> > @Anastasia, what problems do you see if we continue treating LLVM IR with 
> > spir* target triple as LLVM IR representation of SPIR-V format?
> The state of SPIR 1.2/2.0 in Clang seems to be that the SPIR target has 
> transformed to mean “SPIR 1.2/2.0 derivative”, but that does not still make 
> it SPIR-V, which is not based on LLVM IR. When one is targeting spir* there 
> is ambiguity on whether one is aiming to produce the old-SPIR-derivative or 
> SPIR-V. Considering that there are still SPIR-derivative consumers, in my 
> opinion we should have separate LLVM targets for SPIR-V to have explicit 
> disambiguation of intent for producing the SPIR-derivative vs SPIR-V.
@bader, if you would like to migrate SPIR into SPIR-V properly then we should 
at least rename it. I would certainly prefer triple SPIR-V to SPIR which 
eliminates the need to explain 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki updated this revision to Diff 373497.
linjamaki edited the summary of this revision.
linjamaki added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

Files:
  clang/lib/Basic/Targets/SPIR.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(3)* @s to i32 
addrspace(4)*
+  return 
+}
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -56,9 +56,14 @@
 0, // opencl_generic
 0, // opencl_global_device
 0, // opencl_global_host
-0, // cuda_device
-0, // cuda_constant
-0, // cuda_shared
+// cuda_* address space mapping is intended for HIPSPV (HIP to SPIR-V
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in
+// SPIR-V casts between constant and generic pointers are not allowed. For
+// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
+1, // cuda_constant
+3, // cuda_shared
 1, // sycl_global
 5, // sycl_global_device
 6, // sycl_global_host
@@ -219,6 +224,16 @@
   bool hasFeature(StringRef Feature) const override {
 return Feature == "spirv";
   }
+
+  void adjust(DiagnosticsEngine , LangOptions ) override {
+BaseSPIRTargetInfo::adjust(Diags, Opts);
+// Guarded so we don't override address space map setting set by
+// BaseSPIRTargetInfo::adjust.
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.
+  setAddressSpaceMap(/*DefaultIsGeneric=*/true);
+  }
 };
 
 class LLVM_LIBRARY_VISIBILITY SPIRV32TargetInfo : public SPIRVTargetInfo {


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki marked an inline comment as done.
linjamaki added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

bader wrote:
> keryell wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > I am slightly confused as in the LLVM project those address spaces 
> > > > > are for SPIR not SPIR-V though. It is however used outside of LLVM 
> > > > > project by some tools like SPIRV-LLVM Translator as a path to SPIR-V, 
> > > > > but it has only been done as a workaround since we had no SPIR-V 
> > > > > support in the LLVM project yet. And if we are adding it let's do it 
> > > > > clean to avoid/resolve any confusion.
> > > > > 
> > > > > I think we need to keep both because some vendors do target/use SPIR 
> > > > > but not SPIR-V.
> > > > > 
> > > > > So if you are interested in SPIR-V and not SPIR you should probably 
> > > > > add a new target that will make things cleaner.
> > > > > I think we need to keep both because some vendors do target/use SPIR 
> > > > > but not SPIR-V.
> > > > 
> > > > @Anastasia, could you elaborate more on the difference between SPIR and 
> > > > SPIR-V?
> > > > I would like to understand what these terms mean in the context of LLVM 
> > > > project.
> > > Their conceptual differences are just that they are two different 
> > > intermediate formats.
> > > 
> > > The important thing to highlight is that it is not impossible that some 
> > > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > > discontinued by Khronos. 
> > > 
> > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > > Their conceptual differences are just that they are two different 
> > > intermediate formats.
> > > 
> > > The important thing to highlight is that it is not impossible that some 
> > > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > > discontinued by Khronos. 
> > > 
> > > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > 
> > All the official Xilinx OpenCL stack is based on legacy SPIR (encoded in 
> > LLVM 6.x IR but this is another story) and I suspect this is the case for 
> > other companies.
> > So, do not deprecate or discontinue, please. :-)
> > The important thing to highlight is that it is not impossible that some 
> > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > discontinued by Khronos.
> > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> 
> Strictly speaking `SPIR` is not defined as an intermediate language. Khronos 
> defines `SPIR-1.2` and `SPIR-2.0` formats which are based on LLVM 3.2 and 
> LLVM 3.4 version (https://www.khronos.org/spir/). There is no definition of 
> SPIR format based on current version of LLVM IR. Another note is that 
> metadata and intrinsics emitted for OpenCL with clang-14 doesn't follow 
> neither `SPIR-1.2` nor `SPIR-2.0`.
> 
> I always think of LLVM IR as leaving thing that is subject to change by LLVM 
> community, so tools working with LLVM IR must adjust to the particular 
> version (e.g. release version like LLVM 13 or ToT). We apply this logic to 
> SPIRV-LLVM-Translator tool and update it according to LLVM format changes 
> (e.g. kernel argument information defined in Khronos spec must be named 
> metadata whereas clang emits function metadata).
> 
> > I am slightly confused as in the LLVM project those address spaces are for 
> > SPIR not SPIR-V though.
> [skip]
> > Their conceptual differences are just that they are two different 
> > intermediate formats.
> 
> If this is the only difference, I don't think it a good idea to create 
> another LLVM target to separate SPIR and SPIR-V. From my point of view it 
> creates logic ambiguity and code duplication with no additional value. 
> @Anastasia, what problems do you see if we continue treating LLVM IR with 
> spir* target triple as LLVM IR representation of SPIR-V format?
The state of SPIR 1.2/2.0 in Clang seems to be that the SPIR target has 
transformed to mean “SPIR 1.2/2.0 derivative”, but that does not still make it 
SPIR-V, which is not based on LLVM IR. When one is targeting spir* there is 
ambiguity on whether one is aiming to produce the old-SPIR-derivative or 
SPIR-V. Considering that there are still SPIR-derivative consumers, in my 
opinion we should have separate LLVM targets for SPIR-V to have explicit 
disambiguation of intent for producing the SPIR-derivative vs SPIR-V.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:233
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.

Anastasia wrote:
> My guess is that this is not only HIP specific but for example the same 
> applies to SYCL.
> 
> I am not sure if it makes more sense to move this into a `BaseSPIRTargetInfo` 
> since this is not really SPIR-V specific logic. It is just a clang design 
> misalignment between two address space concepts that has to be addressed 
> properly at some point.
> 
The DefaultIsGeneric AS mapping is enabled for SYCL in the 
BaseSPIRTargetInfo::adjust (which also means the mapping is available for both 
the SPIR and SPIR-V targets). On the other hand, the AS mapping for HIPSPV is 
enabled in SPIRVTargetInfo::adjust only as we intend to emit SPIR-V only. I’m 
under the impression that this is what was wanted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

keryell wrote:
> Anastasia wrote:
> > bader wrote:
> > > Anastasia wrote:
> > > > I am slightly confused as in the LLVM project those address spaces are 
> > > > for SPIR not SPIR-V though. It is however used outside of LLVM project 
> > > > by some tools like SPIRV-LLVM Translator as a path to SPIR-V, but it 
> > > > has only been done as a workaround since we had no SPIR-V support in 
> > > > the LLVM project yet. And if we are adding it let's do it clean to 
> > > > avoid/resolve any confusion.
> > > > 
> > > > I think we need to keep both because some vendors do target/use SPIR 
> > > > but not SPIR-V.
> > > > 
> > > > So if you are interested in SPIR-V and not SPIR you should probably add 
> > > > a new target that will make things cleaner.
> > > > I think we need to keep both because some vendors do target/use SPIR 
> > > > but not SPIR-V.
> > > 
> > > @Anastasia, could you elaborate more on the difference between SPIR and 
> > > SPIR-V?
> > > I would like to understand what these terms mean in the context of LLVM 
> > > project.
> > Their conceptual differences are just that they are two different 
> > intermediate formats.
> > 
> > The important thing to highlight is that it is not impossible that some 
> > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > discontinued by Khronos. 
> > 
> > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> > Their conceptual differences are just that they are two different 
> > intermediate formats.
> > 
> > The important thing to highlight is that it is not impossible that some 
> > vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> > discontinued by Khronos. 
> > 
> > Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> 
> All the official Xilinx OpenCL stack is based on legacy SPIR (encoded in LLVM 
> 6.x IR but this is another story) and I suspect this is the case for other 
> companies.
> So, do not deprecate or discontinue, please. :-)
> The important thing to highlight is that it is not impossible that some 
> vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> discontinued by Khronos.
> Nobody has deprecated or discontinued SPIR in the LLVM project yet.

Strictly speaking `SPIR` is not defined as an intermediate language. Khronos 
defines `SPIR-1.2` and `SPIR-2.0` formats which are based on LLVM 3.2 and LLVM 
3.4 version (https://www.khronos.org/spir/). There is no definition of SPIR 
format based on current version of LLVM IR. Another note is that metadata and 
intrinsics emitted for OpenCL with clang-14 doesn't follow neither `SPIR-1.2` 
nor `SPIR-2.0`.

I always think of LLVM IR as leaving thing that is subject to change by LLVM 
community, so tools working with LLVM IR must adjust to the particular version 
(e.g. release version like LLVM 13 or ToT). We apply this logic to 
SPIRV-LLVM-Translator tool and update it according to LLVM format changes (e.g. 
kernel argument information defined in Khronos spec must be named metadata 
whereas clang emits function metadata).

> I am slightly confused as in the LLVM project those address spaces are for 
> SPIR not SPIR-V though.
[skip]
> Their conceptual differences are just that they are two different 
> intermediate formats.

If this is the only difference, I don't think it a good idea to create another 
LLVM target to separate SPIR and SPIR-V. From my point of view it creates logic 
ambiguity and code duplication with no additional value. @Anastasia, what 
problems do you see if we continue treating LLVM IR with spir* target triple as 
LLVM IR representation of SPIR-V format?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Ronan Keryell via Phabricator via cfe-commits]

keryell added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > I am slightly confused as in the LLVM project those address spaces are 
> > > for SPIR not SPIR-V though. It is however used outside of LLVM project by 
> > > some tools like SPIRV-LLVM Translator as a path to SPIR-V, but it has 
> > > only been done as a workaround since we had no SPIR-V support in the LLVM 
> > > project yet. And if we are adding it let's do it clean to avoid/resolve 
> > > any confusion.
> > > 
> > > I think we need to keep both because some vendors do target/use SPIR but 
> > > not SPIR-V.
> > > 
> > > So if you are interested in SPIR-V and not SPIR you should probably add a 
> > > new target that will make things cleaner.
> > > I think we need to keep both because some vendors do target/use SPIR but 
> > > not SPIR-V.
> > 
> > @Anastasia, could you elaborate more on the difference between SPIR and 
> > SPIR-V?
> > I would like to understand what these terms mean in the context of LLVM 
> > project.
> Their conceptual differences are just that they are two different 
> intermediate formats.
> 
> The important thing to highlight is that it is not impossible that some 
> vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> discontinued by Khronos. 
> 
> Nobody has deprecated or discontinued SPIR in the LLVM project yet.
> Their conceptual differences are just that they are two different 
> intermediate formats.
> 
> The important thing to highlight is that it is not impossible that some 
> vendors use SPIR (without using SPIR-V) even despite the fact it has been 
> discontinued by Khronos. 
> 
> Nobody has deprecated or discontinued SPIR in the LLVM project yet.

All the official Xilinx OpenCL stack is based on legacy SPIR (encoded in LLVM 
6.x IR but this is another story) and I suspect this is the case for other 
companies.
So, do not deprecate or discontinue, please. :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:233
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.

My guess is that this is not only HIP specific but for example the same applies 
to SYCL.

I am not sure if it makes more sense to move this into a `BaseSPIRTargetInfo` 
since this is not really SPIR-V specific logic. It is just a clang design 
misalignment between two address space concepts that has to be addressed 
properly at some point.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

bader wrote:
> Anastasia wrote:
> > I am slightly confused as in the LLVM project those address spaces are for 
> > SPIR not SPIR-V though. It is however used outside of LLVM project by some 
> > tools like SPIRV-LLVM Translator as a path to SPIR-V, but it has only been 
> > done as a workaround since we had no SPIR-V support in the LLVM project 
> > yet. And if we are adding it let's do it clean to avoid/resolve any 
> > confusion.
> > 
> > I think we need to keep both because some vendors do target/use SPIR but 
> > not SPIR-V.
> > 
> > So if you are interested in SPIR-V and not SPIR you should probably add a 
> > new target that will make things cleaner.
> > I think we need to keep both because some vendors do target/use SPIR but 
> > not SPIR-V.
> 
> @Anastasia, could you elaborate more on the difference between SPIR and 
> SPIR-V?
> I would like to understand what these terms mean in the context of LLVM 
> project.
Their conceptual differences are just that they are two different intermediate 
formats.

The important thing to highlight is that it is not impossible that some vendors 
use SPIR (without using SPIR-V) even despite the fact it has been discontinued 
by Khronos. 

Nobody has deprecated or discontinued SPIR in the LLVM project yet.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki updated this revision to Diff 371877.
linjamaki added a comment.

Enable HIP-to-SPIR-V address space mapping only for SPIR-V targets.

Patch now depends on D109144 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

Files:
  clang/lib/Basic/Targets/SPIR.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(3)* @s to i32 
addrspace(4)*
+  return 
+}
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -56,9 +56,14 @@
 0, // opencl_generic
 0, // opencl_global_device
 0, // opencl_global_host
-0, // cuda_device
-0, // cuda_constant
-0, // cuda_shared
+// cuda_* address space mapping is intended for HIPSPV (HIP to SPIR-V
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in
+// SPIR-V casts between constant and generic pointers are not allowed. For
+// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
+1, // cuda_constant
+3, // cuda_shared
 1, // sycl_global
 5, // sycl_global_device
 6, // sycl_global_host
@@ -219,6 +224,16 @@
   bool hasFeature(StringRef Feature) const override {
 return Feature == "spirv";
   }
+
+  void adjust(DiagnosticsEngine , LangOptions ) override {
+BaseSPIRTargetInfo::adjust(Diags, Opts);
+// Guarded so we don't override address space map setting set by
+// BaseSPIRTargetInfo::adjust.
+if (Opts.HIP && Opts.CUDAIsDevice)
+  // Enable address space mapping from HIP to SPIR-V.
+  // See comment on the SPIRDefIsGenMap table.
+  setAddressSpaceMap(/*DefaultIsGeneric=*/true);
+  }
 };
 
 class LLVM_LIBRARY_VISIBILITY SPIRV32TargetInfo : public SPIRVTargetInfo {


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spirv64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: define{{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 addrspace(4)*
+  return 
+}
+
+// CHECK: 

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

Anastasia wrote:
> I am slightly confused as in the LLVM project those address spaces are for 
> SPIR not SPIR-V though. It is however used outside of LLVM project by some 
> tools like SPIRV-LLVM Translator as a path to SPIR-V, but it has only been 
> done as a workaround since we had no SPIR-V support in the LLVM project yet. 
> And if we are adding it let's do it clean to avoid/resolve any confusion.
> 
> I think we need to keep both because some vendors do target/use SPIR but not 
> SPIR-V.
> 
> So if you are interested in SPIR-V and not SPIR you should probably add a new 
> target that will make things cleaner.
> I think we need to keep both because some vendors do target/use SPIR but not 
> SPIR-V.

@Anastasia, could you elaborate more on the difference between SPIR and SPIR-V?
I would like to understand what these terms mean in the context of LLVM project.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki planned changes to this revision.
linjamaki added a comment.

Thanks. I will make a patch for adding spirv triples and new target info and 
update this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Basic/Targets/SPIR.h:59
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in

I am slightly confused as in the LLVM project those address spaces are for SPIR 
not SPIR-V though. It is however used outside of LLVM project by some tools 
like SPIRV-LLVM Translator as a path to SPIR-V, but it has only been done as a 
workaround since we had no SPIR-V support in the LLVM project yet. And if we 
are adding it let's do it clean to avoid/resolve any confusion.

I think we need to keep both because some vendors do target/use SPIR but not 
SPIR-V.

So if you are interested in SPIR-V and not SPIR you should probably add a new 
target that will make things cleaner.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clang/lib/Basic/Targets/SPIR.h:146
+// See comment on the SPIRDefIsGenMap table.
+bool IsHIPSPV = Opts.HIP && Opts.CUDAIsDevice;
 // FIXME: SYCL specification considers unannotated pointers and references

Minor: in my opinion, Opts.HIP check is unnecessary. I don't think CUDA can be 
compiled to SPIR target today, but when this flow is enabled I think it should 
set `DefaultIsGeneric` flag the same way as HIP.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108621/new/

https://reviews.llvm.org/D108621

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via cfe-commits]

linjamaki created this revision.
Herald added a subscriber: yaxunl.
linjamaki requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add mapping for CUDA address spaces for HIP to SPIR-V
translation. This change allows HIP device code to be emitted as valid
SPIR-V by mapping unqualified pointers to generic address space and by
mapping __device__ and __shared__ AS to their equivalent AS in SPIR-V
(CrossWorkgroup and Workgroup, respectively).

Cuda's __constant__ AS is handled specially. In HIP unqualified
pointers (aka "flat" pointers) can point to __constant__ objects. Mapping
this AS to ConstantMemory would produce to illegal address space casts to
generic AS. Therefore, __constant__ AS is mapped to CrossWorkgroup.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108621

Files:
  clang/lib/Basic/Targets/SPIR.h
  clang/test/CodeGenHIP/hipspv-addr-spaces.cpp


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spir64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: @s ={{.*}} addrspace(3) global
+__shared__ int s;
+
+// CHECK: @foo ={{.*}} addrspace(1) externally_initialized global %struct.foo_t
+__device__ struct foo_t {
+  int i;
+  int* pi;
+} foo;
+
+// CHECK: define {{.*}} spir_func i32 addrspace(4)* @_Z3barPi(i32 addrspace(4)*
+__device__ int* bar(int *x) {
+  return x;
+}
+
+// CHECK: define {{.*}} spir_func i32 addrspace(4)* @_Z5baz_dv()
+__device__ int* baz_d() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @d to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define {{.*}} spir_func i32 addrspace(4)* @_Z5baz_cv()
+__device__ int* baz_c() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @c to i32 
addrspace(4)*
+  return 
+}
+
+// CHECK: define {{.*}} spir_func i32 addrspace(4)* @_Z5baz_sv()
+__device__ int* baz_s() {
+  // CHECK: ret i32 addrspace(4)* addrspacecast (i32 addrspace(3)* @s to i32 
addrspace(4)*
+  return 
+}
Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -54,9 +54,14 @@
 0, // opencl_generic
 0, // opencl_global_device
 0, // opencl_global_host
-0, // cuda_device
-0, // cuda_constant
-0, // cuda_shared
+// cuda_* address space mapping is intended for HIPSPV (HIP to SPIR-V
+// translation). This mapping is enabled when the language mode is HIP.
+1, // cuda_device
+// cuda_constant pointer can be casted to default/"flat" pointer, but in
+// SPIR-V casts between constant and generic pointers are not allowed. For
+// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
+1, // cuda_constant
+3, // cuda_shared
 1, // sycl_global
 5, // sycl_global_device
 6, // sycl_global_host
@@ -137,6 +142,8 @@
 
   void adjust(DiagnosticsEngine , LangOptions ) override {
 TargetInfo::adjust(Diags, Opts);
+// See comment on the SPIRDefIsGenMap table.
+bool IsHIPSPV = Opts.HIP && Opts.CUDAIsDevice;
 // 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.
@@ -144,7 +151,7 @@
 // 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.
-setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice);
+setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice || IsHIPSPV);
   }
 
   void setSupportedOpenCLOpts() override {


Index: clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
===
--- /dev/null
+++ clang/test/CodeGenHIP/hipspv-addr-spaces.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple spir64 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck %s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK: %struct.foo_t = type { i32, i32 addrspace(4)* }
+
+// CHECK: @d ={{.*}} addrspace(1) externally_initialized global
+__device__ int d;
+
+// CHECK: @c ={{.*}} addrspace(1) externally_initialized global
+__constant__ int c;
+
+// CHECK: