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 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. 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
