aaron.ballman added a comment. In D89909#2623887 <https://reviews.llvm.org/D89909#2623887>, @Anastasia wrote:
> 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. If what you're suggesting is that Clang have a SYCL-specific page that serves a similar purpose to https://clang.llvm.org/docs/OpenCLSupport.html, then I agree we should have that (and it should be linked to from https://clang.llvm.org/docs/index.html the same as OpenCL, OpenMP, etc). However, I think that's orthogonal to this patch and should be done separately. (If the documentation already existed, then I'd request that this patch update it, but because those docs don't exist yet, I think it's unrelated enough to warrant being done separately.) If you're requesting something different, I'd appreciate a bit more specific details. > 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. Given that we lack the dedicated documentation page for SYCL within Clang, I guess I view that as the status quo (we've already started some of the upstreaming work and none of it is documented in a convenient place for Clang users). Based on that, as someone who doesn't really do much with SYCL to date, I think it's reasonable to accept this patch because it extends existing functionality in a straight-forward manner with a reasonable explanation as to why. > Also I am also not entirely happy with some directions taken deviating the > original design in this patch. If there are technical concerns to be addressed, I'm not certain it's clear (to me, at least) what they are. > But it is hard to assess whether there are better practical alternatives and > suggest anything without understanding what is being implemented. That's reasonable, but I think we also have to trust the domain experts who are submitting patches to have done their design homework and to be willing to address concerns as they come up when there's concretely a problem. This is not an extremely experimental invention from an unknown lone contributor; this is a long-time maintainer upstreaming work from downstream repo with users who use this functionality. > 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. Strongly agreed! But my point is: it seems like this is (at least to some extent) already publicly accessible information and I don't think we've identified any disruptions or negative impacts (yet). > 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. I read this as "Maybe it won't take several more months depending on factors outside of the patch author's control" which doesn't really address my concerns that there's not a clear direction on how to unblock this patch in a timely fashion. That's not to imply that I think your concerns are invalid or should be hand-waved away! I agree that we should make sure there's comfort with the design and try to reduce risk as much as we can. It's more that I think I'm recognizing that trying to gain this clarity over Phabricator with long latency on the discussion is somewhat ineffective. Based on the discussion so far, would these be acceptable steps to take? 0) Complete review on this patch for any technical concerns related to it and commit when it's ready (this unblocks some downstream needs quickly, hopefully). 1. @bader (or someone else on the SYCL team) creates a bare-bones SYCL documentation page that is quickly accepted as a placeholder for us to put more documentation. 2. Each new SYCL related review that needs user-facing documentation outside of what the SYCL standard documents will update the Clang SYCL doc. 3. @bader (or someone else on the SYCL team) writes address-space mapping documentation and adds it to the Clang SYCL doc. 4. @bader (or someone else on the SYCL team) looks at other commits that have already gone into Clang to write missing documentation. Some of these steps can be done in parallel, of course. I recognize we could have the order be #1 -> #3 -> #0 (so the docs for this change are written as part of this patch), but my concern with that approach is that this patch is blocking other SYCL efforts (@bader can correct me if I'm wrong about this) and no one has identified a definite issue with it yet beyond the lack of Clang documentation. If those steps seem unreasonable or like I've totally missed the point on something, perhaps we could reach an understanding more quickly via a meeting (we could summarize the decisions from the meeting here so the community is aware of the end results)? 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
