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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
