[PATCH] D89372: [OpenCL] Remove unused extensions

Fri Oct 16 03:12:04 PDT 2020

Anastasia added a comment.

In D89372#2333937 <https://reviews.llvm.org/D89372#2333937>, @jvesely wrote:

> In D89372#2332997 <https://reviews.llvm.org/D89372#2332997>, @Anastasia wrote:
>
>> In D89372#2332853 <https://reviews.llvm.org/D89372#2332853>, @jvesely wrote:
>>
>>> `cl_khr_byte_addressable_stores` changes language semantics. without it, pointer dereferences of types smaller than 32 bits are illegal.
>>
>> Ok, does it mean that we are missing to diagnose this in the frontend? Btw I do acknowledge that what you say makes sense but I don't think the documentation support that:
>> https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_byte_addressable_store.html
>>
>> Am I just looking in the wrong place?
>
> Since it's only an extension in ocl 1.0 that spec is a better place to look:
>
>   9.9 Byte Addressable Stores 
>   Section 6.8.m describes restrictions on built-in types char, uchar, char2, uchar2, short,
>   and half. The OpenCL extension cl_khr_byte_addressable_store removes these
>   restrictions.  An application that wants to be able to write to elements of a pointer (or struct) that
>   are of type char, uchar, char2, uchar2, short, ushort and half will need to include
>   the #pragma OPENCL EXTENSION cl_khr_byte_addressable_store :
>   enable directive before any code that performs writes that may not be supported as per section
>   6.8.m.
>   
>   6.8 Restrictions
>   ...
>   m. Built-in types that are less than 32-bits in size i.e. char, uchar, char2, uchar2,
>   short, ushort, and half have the following restriction:
>   Writes to a pointer (or arrays) of type char, uchar, char2, uchar2, short,
>   ushort, and half or to elements of a struct that are of type char, uchar,
>   char2, uchar2, short and ushort are not supported. Refer to section 9.9
>   for additional information.

Ok, thanks! I guess the documentation should be fixed in this URL to point to the place where this behavior is described, like for example in cl_khr_subgroups:
https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_subgroups.html

>>> Even if all clang targets support this the macro should still be defined for backward compatibility.
>>
>> Ok, are you saying that all targets currently support this and we sould always define it? In this case I would be more happy if we move them into the internal header that we add implicitly anyway...
>
> r600/r700/EG/NI targets have a bit wonky support byte-addressable stores (basically using 32bit atomic MSKOR), but I don't expect those targets to survive for long, and for now, they advertise support.
> some out of tree backends might also benefit from the extension (like legup -- verilog backend), but I'm not sure if out of tree targets are worth considering, or if they indeed make use of this extension.
> on a high-level, I could see a restriction to 32bit data paths be useful for FPGA targets.
>
> moving the define it to header breaks OCL1.0 programs. Based on the specs above. those programs are required to include the following:
>
>   #ifdef cl_khr_byte_addressable_store
>   #pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
>   #endif
>
> Before they can dereference pointers to char/char2/short/... types (including array indexing and struct members).
> so the `pragma` part needs to work, and the language level checks need to work if the extension is not enabled.

Ok, so the pragma is supposed to control the pointer dereferencing which seems like a valid case but however, it is not implemented now. Do we know of any immediate plans from anyone to implement this? If not I would prefer to remove the pragma for now? If someone decided to implement this functionality later fully it is absolutely fine. Pragma will be very easy to add. There is no need for everyone to pay the price for something that can't be used at the moment.

> The same arguments also apply to `cles_khr_in64`. It's illegal to use int64 types in embedded profile unless you first enable the extensions. Rather than removing it, `cles_khr_2d_image_array_writes` should be added to the extension list.

I don't think clang currently support embedded profile. Adding extension into the OpenCLOptions is pointless if it's not used. Do you plan to add any functionality for it? Defining macros can be done easily outside of clang source code or if it is preferable to do inside there is `MacroBuilder::defineMacro`  available in the target hooks. Currently for every extension added into `OpenCLExtensions.def` there is a macro, a pragma and a field in `OpenCLOptions` added. This is often more than what is necessary. Plus Khronos has very many extensions if we assume that all of them are added in clang it will become scalability and maintanance nightmare. Most of the extensions only add functions so if we provide a way to add macros for those outside of clang code base it will keep the common code clean and vendors can be more flexible in adding the extensions without the need to modify upstream code if they need to. I see it as an opportunity to improve common code and out of tree implementations too. It just need a little bit of restructuring.

> clang can always decide that OCL1.0 programs (even when compiled in cl-1.x mode) and embedded profile is unsupported.
> I have no clue if there are many OCL programs that rely on those modes out there.
> However, removing support for them is a significantly bigger change than cleaning up a few language-irrelevant extensions (actually, I'm not sure if clang ever supported OCL embedded mode).

I don't think so. There is only one extensions added but that hasn't been implemented fully. As a matter of fact I have created this review earlier to make it clear in the User Manual: https://reviews.llvm.org/D89143. Feel free to provide your feedback.

>>> it would be useful if the commit message or the description of this revision included a justification for each removed extension why it doesn't impact language semantics with spec references.
>>
>> Yes, this is a good suggestion in principle and we should try our best. However I am not sure it is feasible for all of those, in particular this documentation doesn't contain anything:
>> https://man.opencl.org/cl_khr_context_abort.html
>>
>> Are you suggesting to leave this out? However I don't see any evidence that this require either macro or pragma. I feel this is in rather incomplete state. So I don't feel we can accomodate for all of these.
>
> "incomplete specification" is imo a good reason to drop support for an extension. My argument is that explanation of legacy extensions should rely on the spec that introduced them, rather than the current 2.x/3.x which does an arguably poor job on backward compatibility.

Ok, the idea is not to break backwards compatibility of course. For the extensions that intended to modify language (but never did) if we look from upstream clang user's perspective those extensions couldn't possibly be used to do anything useful. It is of course possible that in some forks the functionality has been completed but then they can easily update the implementation to include the extension definition back. This is very small change compared to the actual extension functionality.

I am ok to leave the extensions that could hypotetically modify the language out of this patch for now. Perhaps we could add a comment explaining that they are unused and only left for backwards compatibility. In a long term we need to find some ways to remove the legacy that doesn't bring any benefit to the community. Maybe I will write another RFC and ask vendors to reply in the mainling list if they use those and plan to either complete the functionality upstream or remove them.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372