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 > > > > > > > > > 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... > > > > > > 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... > > Does this patch break any ABIs for OpenCL? I think it's specific to HIP/CUDA > language and doesn't impact OpenCL compiler. Please, let me know if I get it > wrong. > > I fully support improving tooling for SPIR-V, but in my opinion some of such > improvements should be done separately from the work done by Henry as they > require additional discussions. I feel that our discussion has diverged from this patch as right now we are discussing how to add SPIR-V target while this patch needs a change of address spaces for SPIR-V. > I fully support improving tooling for SPIR-V, but in my opinion some of such > improvements should be done separately from the work done by Henry as they > require additional discussions. I don't think we can or even should add everything in one commit. The improvements we can do however will likely depend on the route that is taken. 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
