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