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

Fri Mar 12 12:34:01 PST 2021

Anastasia added a comment.

In D89909#2617194 <https://reviews.llvm.org/D89909#2617194>, @bader wrote:

> In D89909#2606180 <https://reviews.llvm.org/D89909#2606180>, @Anastasia wrote:
>
>> In D89909#2600859 <https://reviews.llvm.org/D89909#2600859>, @aaron.ballman wrote:
>>
>>> Just a few minor nits from me, but I'm mostly wondering: where are we at with this and are there still substantive changes required? (I looked through the comments, but there's a lot of back-and-forth since Oct and I'm not certain what's holding the patch back currently.)
>>
>> To make it short, from my side I am not very clear about the overall design. From the SYCL spec side, there is no indication of what compiler extensions are needed and if at all. As a result, some of the design choices are unclear to me - in particular why SPIR target would need a separate address space map for SYCL. This is not how it was intended originally and I am worried that this will create issues for the consumers of IR to handle two different formats. But in general, if the community is now to maintain this code we should at least have some deeper understanding of it.
>>
>> I would suggest starting from some high-level documentation that provides the details of the compiler extension being implemented. Perhaps the documentation that @bader has linked earlier could be used as a starting point with some more details that would allow assessing and reviewing the changes.
>
> @Anastasia, do you suggest we copy https://github.com/intel/llvm/blob/sycl/sycl/doc/CompilerAndRuntimeDesign.md document to clang/docs within this patch?

For the purpose of this specific topic, you could indeed move "Address spaces handling" section into the clang documentation. I would suggest creating a dedicated page where you could gather SYCL specific internals for the clang community. You could also link the github page to it for more comprehensive details.

I would recommend extending the documentation slightly to focus on what behavior is expected to be implemented rather than how you propose to implement it in clang too. This is a general guideline for the clang contributions that suggest that the documentation should be detailed such that it would be possible to implement it in other frontend/compiler. You could for example include some more information on expected language semantic i.e. what you inherit from embedded C and what you inherent from OpenCL or any other available language features with relevant spec/doc references including SYCL spec. This should facilitate anyone who needs to understand the implementation to find further details.

It would probably make sense to create a separate review to avoid adding too much noise here. Once you create a review we can link it here and also refine any necessary details as we go along.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909