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

Fri Mar 12 12:52:54 PST 2021

aaron.ballman added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909