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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Sep 21 03:36:04 PDT 2021


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 what it actually is and especially considering that SPIR has existed as an alternative IR format for quite a while. It would at least make sense tpo eliminate the confusion.

However if you would like to go this route you should send a wider community messaging about it and then see if there are any objections. From my experience of previous conversations some years back there are tool developers using SPIR as a portable format even if it's LLVM release dependent however in practice it worked across the latest releases quite well. I would like to remind that not all vendors that support OpenCL or other accelerator API also support SPIR-V. There are also vendors that are migrating to SPIR-V but have older releases in maintenance that don't support SPIR-V. So my feeling is that SPIR has been and is still used as a portable format in tooling.

Regarding an extra triple/target, I don't see a lot of code duplication if we use inheritance/generic programming and other C++ features that will allow us to share the code effectively between both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108621



More information about the cfe-commits mailing list