[PATCH] D89372: [OpenCL] Remove unused extensions

Fri Oct 16 06:34:27 PDT 2020

jvesely added a comment.

In D89372#2334225 <https://reviews.llvm.org/D89372#2334225>, @Anastasia wrote:

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

I though the current behaviour of 'you can use #pragma, but the dereferences are always legal' was intentional for backward compatibility.
I don't think there are programs that would set it to 'disabled' and expect compilation failure. My concern is that legacy apps would set '#pragma enabled' before using char/short. such behavior would produce warning/error if with this change applied.

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

My understanding is that both the macro and working #pragma directive is necessary. The configuration bit is only needed if clang changes behaviour, which is a separate question.
I'd also argue that new third party extensions need an API call to register new extensions in order to get a working pragma mechanism.

Even if an extension only adds access new functions, pragma should control if user functions with conflicting names are legal or not.

for example, a program can implement function `new_func`, which gets later added to an extension. since the program doesn't know about the new extension it doesn't use `#pragma new_extension:enable` and continues to work correctly.
If the new extension exposes `new_func` unconditionally, the program breaks, because it doesn't check for a macro that didn't exist at the time it was written.
more recent programs can use ifdef to either use `new_func` provided by the extension, or implement a custom version.

I didn't find much about embedded program behavior in full profile implementation in the specs.
It only says that "embedded profile is a subset" which imo implies that legal embedded profile programs should work correctly in a full profile implementation.
This implies that cles_* pragmas should continue to work even if the behavior is always supported.

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

I'd phrase it differently. extensions that modify the language should be included even if clang decides not to implement given language modification.

I think it's perfectly fine to always allow dereferencing of char/short types, as long as correctly written CL1.0 programs that use `#pragma OPENCL cl_khr_byte_addressable_store: enable` don't produce extra errors/warnings.
Full profile is a superset of embedded profile so the cles_* behaviours should be available and #pragma should work as well.

This would suggest that there should be a list of no-op extensions that are only present for backward/embedded compatibility and don't change behaviour since the extended version of language behaviour is always supported.

note that my concerns are purely wrt. spec wording and intentions. Given that ocl didn't see wider use until later versions, such legacy applications might not exist.
feel free to reject them as purely academic, but the point is that changes in this revision break backward compatibility (which is always a maintenance burden).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372