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

Sat Mar 13 17:46:04 PST 2021

Anastasia added a comment.

> 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 don't really mind the format. It doesn't have to be in Clang documentation although perhaps it makes the most sense logically. What I would like to see is a transparancy within the commnity that is I believe a common practice as per clang contribution policy:

  A specification: The specification must be sufficient to understand the design of the feature as well as interpret the meaning of specific examples. The specification should be detailed enough that another compiler vendor could implement the feature.

I don't find it great to continue reviewing the work where I don't have a clear picture. I also would like to avoid the situation the community has to work with code that we don't understand. We should not be in need to reverse engineer what is being implemented.  It is not a comfortable position to be nor it is productive. Once we know the high-level language semantic it facilitates us to reason about implementation should we need to redesign anything or fix a bug. If we only have fragements of code it doesn't tell us a lot and there is very little we can do with it.  Perhaps this is why good practices have been added by the community? There are also other areas whether it demonstrated to work really well - for very good reasons we no longer add undocumented attributes to clang.

> 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.

I am not aware of all work on SYCL, but I believe some documentation was provided, for example, for the new kernel attribute?

Personally I don't find the patch straight-forward mainly because it deviates from the original design wrt target address space map. But there are other concerns - for example the need for `InferAddressSpace` that is not required for embedded C and C++ functionality that to my understanding SYCL aims to implement. However, I don't know whether my understanding of the aim is correct as I don't have any reference to verify my understanding.

> If there are technical concerns to be addressed, I'm not certain it's clear (to me, at least) what they are.

Well the issue I got stuck at was highlighted earlier - the need to add new address spaces map for SYCL. So far the design we were striking at is that every language would have a new entry in the map but adding multiple maps per language seems to defeat the purpose. Considering that OpenCL and SYCL would likely be supported by similar targets we should try to avoid this or at least try to understand why we ended up with such situation and what we might be able to do to improve. I would accept if we can't do anything with this immediately in case it requires big restructuring but perhaps at least this deserves a wider community communication.

> 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.

I don't think this can be called a trust issue. For me personally, it is about making sure to align with the requirements from various domains that none of us has complete knowledge of. There is certainly no invention but however a case of substantially new functionality that is using the feature scattered all over the frontend source code. I am sorry that I just don't find it trivial. While I have worked on this topic for many years I wouldn't trust myself to make a new design decision alone because it is important to get things right in general not just for my product. As a matter of fact me and some other developers from this area were working quite a lot in the last years on the unification of the functionality for different dialects which results in better understanding and less maintenance.

> 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.

This is somewhat unfortunate because the fact that collaboration would be needed in this area has been highlighted straight from the beginning. Technically  we would only need #3 to unblock #0 as creating a new page is very trivial and it could be done as a part of documenting the SYCL address space language sematic. I believe that with the use of references to existing documentations - SYCL, OpenCL, embedded C specifications, etc, such a goal can be accomplished without significant time investment.

> 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)?

I am not opposed to this, but it would be good to agree on the exact agenda before to ensure we can make some progress.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909