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

Fri Mar 12 17:59:29 PST 2021

Anastasia added a comment.

In D89909#2623250 <https://reviews.llvm.org/D89909#2623250>, @aaron.ballman wrote:

> In D89909#2623211 <https://reviews.llvm.org/D89909#2623211>, @Anastasia wrote:
>
>> In D89909#2617194 <https://reviews.llvm.org/D89909#2617194>, @bader wrote:
>>
>>> @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.
>
> These all seem like good suggestions, from my limited perspective, so thank you for them! I'm not opposed to the documentation requests, however, I think that is work that is separable from the proposed changes in this review and could be done with post-commit follow-ups, unless you think the current documentation is wrong as it relates to this patch. I worry that back-and-forth on documentation clarity (while certainly important) is going to hold this patch up for several more months, which is a burden.

Just to be clear I am not suggesting a nice-to-have documentation. I would like to see the documentation that explains what behavior is expected from the compiler. I think this is normally a good starting point and it has been a common practice for many contributions I believe too. I guess we could first commit the implementation and then work out what we want it to be doing but that implies a bigger risks to the community. Also I am also not entirely happy with some directions taken deviating the original design in this patch. But it is hard to assess whether there are better practical alternatives and suggest anything without understanding what is being implemented.

Address spaces are used by many stakeholders in clang so having a clear understanding of various use cases publicly accessible would benefit the community and reduce the risks of disruptions and negative impact of each other's work.

Regarding the timing I believe it could take less than several months considering that there are some parts already available but it depends on various factors including everyone's availability. However, the fact that some areas would require collaborative exploration of design alternatives from stakeholders was to my memory highlighted on the original proposal to contribute SYCL support to clang quite a while ago. So the requirement here should not appear as unexpected.

This has been my suggestion to address the ping-ponging discussion issues and latencies/time incurred by them as I think it would be good for us to get the terminology sorted. But I am happy if there are other suggestions addressing the concerns.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909