bader marked an inline comment as done. bader added a comment. 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. >>> 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. > 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. >>> 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. > `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). 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. 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
