[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