[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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

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

The initial state of the pragma is disabled, so if disabling the extension isn't supposed to do anything then I don't know why would anyone ever enable it?

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

Correct, it will compile with a warning but not fail to compile. I am willing to introduce a -W option (if not present already ) to suppress that warning if it helps with the clean up and backward compatibility. It might also open up opportunities for a wider clean up  - removing pragma in extensions that currently require pragma for no good reason.  I have written more details on this in https://github.com/KhronosGroup/OpenCL-Docs/pull/355#issuecomment-662679499

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

I don't believe this is the correct interpretation. If you look at the extension spec s9.1 it says:

`Every extension which affects the OpenCL language semantics, syntax or adds built-in functions to the language must create a preprocessor #define that matches the extension name string.  This #define would be available in the language if and only if the extension is supported on a given implementation.`

It does not say that the pragma is needed, it only says that macro is needed. That perfectly makes sense because the macro allows to check that the extension is present to implement certain functionality conditionally.

OpenCL spec however never clarified the use of pragma that technically makes sense because the pragmas in C languages are used for altering the standard behavior that can't be otherwise achieved using standard parsing i.e. C99 section 6.10.1 says about non-STDC pragmas:

`The behavior might cause translation to fail or cause the translator  or  the  resulting  program  to  behave  in  a  non-conforming  manner.   Any  such pragma that is not recognized by the implementation is ignored.`

So C99 only regulates behavior of STDC pragmas and for those it explicitly says how the behavior of the parsed program is altered.

Technically OpenCL pragmas are not covered neither in OpenCL C and not C99 and therefore it is unclear what the implementation should do. However, with time due to the absence of the clarification mutiple interpretations appeared. Sadly some of them ended up in a very suboptimal state both for the tooling and the application developers because the pragma started to be added for no reason or for controlling redundant diagnostics of use of types or functions that extensions were introducing. If you are interested in more details I suggest to check this issue: https://github.com/KhronosGroup/OpenCL-Docs/issues/82

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

Can you explain this please?

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

This is absolutely not how it works or what the pragma currently does or even can do. If you want such a thing to work you need to introduce a concept of a namespace into OpenCL C, so every extension is implemented as a separate namespace to avoid name clushing. I do acknowledge however that it would be extremly useful and convenient but it is not how C parsers work. And I imagine it is not trivial to extend this but probably not impossible.

https://godbolt.org/z/d8c5fb

If you look at this example the function from the extension have already been added into the list the symbal tables. The pragma doesn't undo or remove them or make them invisible. There is no machnism to do this easily and the proper solution would be to use namespaces but it is only available in C++.

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

Yes, this is the intent of defining the extension macros.

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

If there is no proper specification for this I think adding it to clang contradict the development policy http://clang.llvm.org/get_involved.html

  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.



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

Why do we need to add what we don't implement?  I don't think this complies to the clang development policy either as we don't support incomplete implementations.

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

What do the extensions do? If they do nothing why do they need to be in clang? Once again if anyone needs to define a macro, this can be done outside of clang code base.  OpenCLExtensions.def is not meant to be used for adding the macros as it does a lot more than this. Using `OpenCLExtensions.def` for only for adding a macro is a hack to me.

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

I disagree on that there are more OpenCL 1.x conformant devices than OpenCL 2.x. You can see by the adoption list.

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

I think we already agreed earlier to leave certain extensions in for now but add a comment to address this later. See:

  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