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

[Alexey Bader via Phabricator via cfe-commits]

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