Anastasia added a comment. In D89909#2536331 <https://reviews.llvm.org/D89909#2536331>, @bader wrote:
> In D89909#2536157 <https://reviews.llvm.org/D89909#2536157>, @Anastasia wrote: > >> In D89909#2534790 <https://reviews.llvm.org/D89909#2534790>, @bader wrote: >> >>>> Regarding SYCLDevice and SYCLAddrSpaceMap I am still not very convinced >>>> about the flow. Have you had any design discussion regarding this already >>>> that you could point to? >>> >>> We discussed this with you in https://github.com/intel/llvm/pull/1039/. >> >> I can't see. > > The mapping has been discussed in this comment: > https://github.com/intel/llvm/pull/1039/#discussion_r369667791. The discussion you cite is in the separate Github project, but you should have a discussion using LLVM channels about the design you propose for llvm-project repo. Also I don't think citing resources with many comments and no conclusions is making the review process very productive. >>>> My main concern is that I am not sure it fits with the main design of >>>> Clang - producing IR that is to be consumed by multiple targets rather >>>> than only one specific target or even an external tool. Why do you add a >>>> triple component that is used only for one target SPIR: >>>> >>>> How would existing toolchains that consume LLVM-IR for SPIR cope with the >>>> fact that the Default address space is different for C++ and SYCL. >>> >>> High level languages differences doesn't matter for toolchains consuming >>> LLVM IR - it can python, D or SYCL. Toolchains are assuming that LLVM IR >>> follows the sematic defined in SPIR-1.2 document - >>> https://www.khronos.org/registry/SPIR/specs/spir_spec-1.2.pdf. >> >> I can't understand what you mean by "doesn't matter"? > > LLVM has it's own sematic which toolchains rely on. Toolchains consuming LLVM > IR doesn't know if this LLVM IR was emitted by C++ compiler or written > manually - they can rely only on LLVM IR semantic defined by > https://llvm.org/docs/LangRef.html. > >> If you compiler from C++ you get one address space as `Default` and if you >> compile from SYCL you get a different one for the same target - how do you >> expect the toolchain consuming the IR to handle that? > > Technically clang changes SPIR target mapping for Default address space only > for `sycldevice` environment component of the target triple, so it doesn't > depend on the input language i.e. Default is mapped to the same LLVM address > space for C++ and SYCL. If you are suggesting to add SYCL as a component for other languages supported by clang this deserves a separate review probably with different reviewers and a wider discussion with the community. >> FYI, the document is no longer describing the current SPIR target adequately >> as implementation deviated quite a bit from the original documentation. > > This is unfortunate. Do you know if there are any plans to provide up-to-date > documentation? It's critical for non-clang front-ends targeting SPIR-V format. I am not aware of any plan. FYI SPIR has not been added or designed for SPIR-V. It is only used for SPIR-V as a workaround by some external projects but it is not the only use case even if perhaps an important one for the community. >>>> Why is changing of `the address space map necessary for SPIR but not the >>>> other targets? >>> >>> The main reason is the difference in handling Default address space in C++ >>> for OpenCL mode and SYCL. C++ for OpenCL doesn't always deduce address >>> space and there are cases when Default is used and SPIR target maps it AS 0 >>> (equivalent of SPIR-V Function storage class). This lead to inconsistencies >>> like reported here: https://bugs.llvm.org/show_bug.cgi?id=45472. >>> >>> SYCL doesn't deduce language address space at all relies on mapping Default >>> in LLVM AS equivalent of SPIR-V generic. >>> >>> NOTE: other GPU targets like AMDGPU and NVPTX do the same i.e. maps Default >>> language AS to LLVM IR AS equivalent of SPIR-V generic, so there is no need >>> to update. I believe SPIR target must apply the same mapping. >> >> Generic address space in Clang is a logical concept added for OpenCL to >> implement the following logical concept . >> https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#the-generic-address-space >> >> Targets are allowed to map it to any physical address space (it is very >> reasonable to map it to `Default`) but it is only used for restricted use >> cases i.e. pointers or references. > > I agree with that, but I don't get how it's related to the mapping language > AS to LLVM AS. Every language address space has its own semantic that should be mapped appropriately to LLVM IR to make sure the semantic is preserved. OpenCL generic is a concept governed by OpenCL C spec cited above It can be used with pointer types and it represents a placeholder for any of the named address spaces - global, local or private. It signals that a pointer points to an object in one of these concrete named address spaces. The exact address space resolution can occur dynamically during the kernel execution. and it is completely different to the concept of `Default` that is a flat address space of C and C++ as per Embedded C s5.1.1 For the purpose of this Technical Report, the C language is extended to support additional address spaces. When not specified otherwise, objects are allocated by default in a generic address space, which corresponds to the single address space of ISO/IEC 9899:1999. In addition to the generic address space, an implementation may support other, named address spaces. I hope you see the difference between the two. As SPIR is a virtual target, `Default` and `OpenCL generic` have distinct values because you need to preserve their semantic until the mapping to the physical device. I don't see why these two concepts should have the same logical mappping? For the generality tools consuming SPIR LLVM IR should have information for both address spaces and then decide how to represent or map those to a physical device. Then on a physical device target it makes sense that some or even all address spaces are mapped to the same value representing physical segments. >> `Default` address space is where objects are put by default - for example in >> C++ everything is mapped to this address space. I don't see why you just >> replace `Default` by OpenCL generic. It has been added for a very different >> purpose. > > SPIR-V Generic is the only address space allowing us to preserve correct > semantic of the program (possibly `global` can be used, but we didn't > investigate this option due to obvious performance implications). FYI LLVM doesn't support SPIR-V yet, but nevertheless SPIR-V generic has the same semantic as OpenCL generic but it is not the same as `Default` in clang. > The objects from `Default` AS might be allocated in any physical AS, so LLVM > IR AS 4 allows LLVM compiler to infer it using `InferAddressSpace` pass. As far as I am aware you can also use any value with `InferAddressSpace` pass not only 4. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89909/new/ https://reviews.llvm.org/D89909 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
