[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.

>> 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"? 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? Certainly this is not how LLVM was designed. LLVM expects the same address spaces to be used with one target for all languages.

FYI, the document is no longer describing the current SPIR target adequately as implementation deviated quite a bit from the original documentation.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909