[PATCH] D71005: [AST] Enable expression of OpenCL language address spaces an attribute

Thu Dec 5 08:19:03 PST 2019

aaron.ballman added a comment.

In D71005#1770760 <https://reviews.llvm.org/D71005#1770760>, @bader wrote:

> >>> is there a reason this should be clang::opencl_private as opposed to opencl::private?
> >> 
> >> I'm okay with [[opencl::private]] as well. I have only one problem - currently OpenCL address spaces are exposed as keywords and using them in C++ breaks valid C++ code.
> > 
> > I'm not certain who controls the OpenCL spec, but this seems like it should be a decision that comes from there. Or is this functionality outside of the OpenCL spec?
>
> I guess I owe you some background here. OpenCL spec is controlled by Khronos organization, but Khronos provides only C-based language specification "OpenCL C" and I think this functionality is outside of the OpenCL spec.
>
> Today Khronos organization rely on SPIR-V standard to enable high-level languages. This standard defines binary form of low-level intermediate language, which can be targeted by high-level language compiler like C++/Python/Java/Haskel/etc. High level level languages usually have separate working groups or standardization committees defining language features.
>
> In addition to that SYCL working group within Khronos organization defines a C++-based abstraction interface <https://www.khronos.org/sycl/> to program accelerators like GPU, FPGA, DPS. SYCL interfaces do not expose any new C++ keywords or attributes and can be implemented as library using standard C++, but this implementation won't be able to offload execution of C++ code to an accelerator as standard C++ do not provide required facilities. To enable execution on accelerators, we implemented "SYCL device compiler" mode in clang and enhanced SYCL library with a few non-standard C++ extensions (e.g. attributes). Our implementation targets SPIR-V format for accelerated code and OpenCL API to communicate with accelerators. AFAIK, Codeplay team uses CUDA to enable execution of the SYCL code on NVIDIA GPUs <https://github.com/intel/llvm/issues/879#issuecomment-560453423>.
>
> In our implementation we are trying to re-use existing clang infrastructure developed for other GPU programming models OpenCL/OpenMP offload/CUDA/HIP, but due to design differences it can't always be re-used as is. For instance, if I understand it correctly, we can't use OpenCL address space attributes exposed as keywords as they might break valid C++ code - https://godbolt.org/z/fF5Ng5.
>  That's the basic motivation for this change.

Thank you for the background! Now for some background of my own. :-)

Double square bracket-style attributes (which are supported in both C and C++) can only have a single level of vendor namespacing. So we can go with `clang::` or `opencl::` but cannot go with `clang::opencl::`. We control the `clang` vendor namespace for attributes, but we do not control the `opencl` vendor namespace (if one even exists). However, these attributes seem like they don't really "belong" to clang because they're OpenCL-specific moreso than clang-specific. That's why I was asking if it would make more sense to put them under an `opencl` namespace -- this allows Microsoft or GCC (or whatever other implementation) to implement the same semantics for their OpenCL implementations but without requiring them to introduce our vendor namespace into their products. However, I wasn't certain what entity could make decisions about that vendor namespace. Based on what you wrote, it sounds like the Khronos group is the most likely one to want to control that namespace, but I'm not certain.

To be clear, there's nothing invalid or incorrect about `clang::opencl_whatever`, it just looks strange because that's basically adding a second-level namespace.

>> In attributes, when an identifier can be interpreted as a keyword it is required to be interpreted as an identifier. We have the same issue with [[gnu::const]]. See http://eel.is/c++draft/dcl.attr.grammar#4.sentence-5
> 
> If I understand it correctly, it's the reason why `const` attribute has "GCC" spelling, instead of "Keyword" here https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Attr.td#L940. Right?

I may not have been sufficiently clear. I was talking about attributes within double square brackets, like `[[gnu::const]]` -- that `const` is interpreted as an identifier rather than a keyword, making this a legal attribute in C and C++. So there's nothing wrong with `[[opencl::private]]` as an attribute, despite `private` being a keyword. I think you're talking about turning the keyword-like attribute `__private` into `private`, then you're correct that we do not have a way to support something like that.

>> will other implementations of OpenCL want to have the same functionality? Or is this something we expect other OpenCL implementations to largely ignore?
> 
> Exposing these attributes as non-keywords is useful for SYCL implementations. Actually this patch was developed by Codeplay team for ComputeCPP compiler - another SYCL implementation and "donated" by @Naghasan to our open source implementation to avoid implementation divergence.
> 
> To summarize: SYCL implementation need some instruments to express OpenCL address spaces in C++ code w/o breaking existing C++ code. We find that attributes fit well for this use case, but I'm open for alternative ideas.

I think attributes are a great approach to solving the problem. I'm only pushing back on the names of the attributes and whether we think they should be in the `clang` vendor namespace or an `opencl` one. Now that C2x has support for attributes, I would think that Khronos could be approached to see if they would like to see [[opencl::private]] as an attribute in C that could then also be used with the same semantics in a C++ program. WDYT? If they wanted to "own" the `opencl` vendor namespace, that would make me more comfortable adding a new attribute under that namespace. If they don't want to own that namespace, or if they don't want to use the attribute syntax in C at all, that's also fine -- we can go with what's already committed in the `clang` vendor namespace.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71005