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

Wed Feb 3 08:09:00 PST 2021

bader added a comment.

> Anastasia added a comment.
> In D89909#2536331 <https://reviews.llvm.org/D89909#2536331> https://reviews.llvm.org/D89909#2536331, @bader wrote:
>
>> In D89909#2536157 <https://reviews.llvm.org/D89909#2536157> https://reviews.llvm.org/D89909#2536157, @Anastasia wrote:
>>
>>> In D89909#2534790 <https://reviews.llvm.org/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.

Sorry, I forgot to mentioned that this change was also discussed in the mailing list: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-td4068754.html.
There are no objections from the community.

What is not covered by this thread and probably worth to discuss separately is changing `OpenCL private` mapping to non-zero LLVM AS to avoid the mangler component change. I expect significant impact on SPIR LLVM IR consumers and long transition period if the change is accepted.

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

No, but clang doesn't couple input language with environment component of the target triple. As I mentioned above, community doesn't have any objections to this approach.

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

I'm aware of SPIR target history and design - I was on the team contributed SPIR-1.2 support to clang, although I was involved into compiler back-end development.
We designed and contributed SPIR-1.2 support to clang aiming to provide a tool producing stable portable binary format for OpenCL programs.
SPIR format evolved from SPIR-1.2 to SPIR-V, which is not based on LLVM-based, but AFAIK the goal of SPIR target in clang tool has not changed and it is still to produce binary format for Khronos APIs (even though part of the toolchain is hosted outside of LLVM project).
It's great if there are other uses of SPIR target in LLVM project. Do you know where I can learn more about this?

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

I puzzled by existing mapping: Aren't `Default` and `SPIR-V Function` (or `OpenCL private`) semantically different address spaces?
I don't see what difference between `Default` and `SPIR-V generic` (or `OpenCL generic`) plays significant role at LLVM IR level. 
Note that LLVM IR objects are not allocated neither in `Default` nor in `Generic` address spaces as described by John in this message <http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-tp4068754p4069039.html>.
This seems to be only property important for separate mapping of these two address spaces to LLVM IR.
Could you provide an example where mapping `Default` to `SPIR-V generic` might cause the problem, please? I have a problem with imagining such case, but I can be biased.

FWIW, real GPU targets map `Default` to `Generic` and as far as I concerned it doesn't cause problems.

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

Right, but we use SPIR target and it uses 4.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909