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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 5 04:19:21 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
----------------
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* 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.
> > > >  if you would like to migrate SPIR into SPIR-V properly then we should at least rename it. 
> > > 
> > > I have an impression that existing SPIR target should work for both use cases: tools working with "SPIR 1.2/2.0 derivatives" and LLVM -> SPIR-V translation tool(s). I'm trying to clarify why adding mapping for CUDA address spaces works for SPIR-V, but doesn't work for "SPIR 1.2/2.0 derivatives".
> > > I have an impression that existing SPIR target should work for both use cases: tools working with "SPIR 1.2/2.0 derivatives" and LLVM -> SPIR-V translation tool(s).
> > 
> > Ok, I have two concerns if we take this route:
> > 1. What do we do about documentation and messaging if we use one target for both? I imagine some updates will be needed somewhere to make it clear that SPIR is SPIR-V and SPIR-V is SPIR and that they will evolve the same way if we decide to go this route... Then at least we probably need a new triple for SPIR-V?
> > 2. What happens if we need different behavior for SPIR-V than what SPIR currently has? For example, my impression is that for SPIR-V backend some OpenCL builtins will be represented differently. Btw developers working on SPIR-V backend should probably be included into this discussion...
> > 
> > Overall I feel adding a new target with code reuse from SPIR will probably make things clearer in a long run, but this should probably be discussed elsewhere either in https://reviews.llvm.org/D109144 or as a wider discussion perhaps via a new RFC thread about the best approach of adding SPIR-V target and the future evolution of SPIR. Then we can make sure this can reach the right audience... Then we can collect a list of requirements about different use cases that developers targets and where they are heading with those in the future and define a suitable direction.
> > > I have an impression that existing SPIR target should work for both use cases: tools working with "SPIR 1.2/2.0 derivatives" and LLVM -> SPIR-V translation tool(s).
> > 
> > Ok, I have two concerns if we take this route:
> 
> This route has been taken starting with LLVM 3.4+ after SPIR switched from LLVM-based format to SPIR-V, so adding another target and deviating LLVM IR format for SPIR-V from "SPIR 1.2/2.0 derivatives" can be disruptive for the tools like SPIR-V translator. How do you see the transition for these tools to LLVM IR for another target?
> 
> > 1. What do we do about documentation and messaging if we use one target for both? I imagine some updates will be needed somewhere to make it clear that SPIR is SPIR-V and SPIR-V is SPIR and that they will evolve the same way if we decide to go this route... Then at least we probably need a new triple for SPIR-V?
> 
> I'm not sure if there is a confusion about the difference between LLVM IR for SPIR target and SPIR-V format. As noted above, SPIR target has been used "for both" from the start (i.e. as soon as SPIR-V has been introduced). Additional SPIR-related restrictions/additions for LLVM IR format are not documented anywhere (except a [[ https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst#additional-requirements-for-llvm-module | short section ]] in the SPIR-V translator documentation), so it seems to be a good idea to document the format and how to use it (e.g. https://llvm.org/docs/AMDGPUUsage.html).
> 
> > 2. What happens if we need different behavior for SPIR-V than what SPIR currently has? For example, my impression is that for SPIR-V backend some OpenCL builtins will be represented differently. Btw developers working on SPIR-V backend should probably be included into this discussion...
> 
> OpenCL defines built-ins representation in high-level language and SPIR-V defines it for the binary format. How built-ins are represented in LLVM IR is not defined, so implementers has freedom to design it. I think SPIR-V backend developers are trying to design it so multiple languages can target SPIR-V format in addition to OpenCL.
> 
> > 
> > Overall I feel adding a new target with code reuse from SPIR will probably make things clearer in a long run, but this should probably be discussed elsewhere either in https://reviews.llvm.org/D109144 or as a wider discussion perhaps via a new RFC thread about the best approach of adding SPIR-V target and the future evolution of SPIR. Then we can make sure this can reach the right audience... Then we can collect a list of requirements about different use cases that developers targets and where they are heading with those in the future and define a suitable direction.
>         I have an impression that existing SPIR target should work for both use cases: tools working with "SPIR 1.2/2.0 derivatives" and LLVM -> SPIR-V translation tool(s).
> 
>     Ok, I have two concerns if we take this route:
> 
> This route has been taken starting with LLVM 3.4+ after SPIR switched from LLVM-based format to SPIR-V, so adding another target and deviating LLVM IR format for SPIR-V from "SPIR 1.2/2.0 derivatives" can be disruptive for the tools like SPIR-V translator. How do you see the transition for these tools to LLVM IR for another target?

My understanding is that the tools were designed with reuse of SPIR target because we haven't been able to add SPIR-V target into LLVM. If we were able to do it earlier I am not sure that it would have been done this way.

At this point I would like to draw attention to the fact that in OpenCL we would like to revise and improve the tooling for SPIR-V in comparison to what they were in SPIR. One example is a redesign of builtin function support. However there are a lot of tools that do rely on SPIR target and changing the design for SPIR would cause ABI changes for them which we would like to avoid. So at least in OpenCL we would need to maintain old SPIR format but also migrate to more optimal SPIR-V tailored tooling support. In general, I see adding SPIR-V target explicitly as an opportunity to reset and optimize tooling architecture...




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