[PATCH] D89372: [OpenCL] Remove unused extensions

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

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

> 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

Adding a new option cannot be a solution as legacy applications were written before its introduction.
CL1.x applications need to continue to work without changes, anything else breaks backward compatibility.

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

This is only one part indicating the availability of extension. It's not describing what's necessary to observe extended behaviour. The specs state that the initial state of the compiler is as if `#pragma OPENCL EXTENSION all : disable` was issued.
This means that functions introduced by extensions cannot be present, and the symbol names are available for application use unless such symbols were previously reserved.

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

This cited line only talks about extensions availability, not extension use. One example of newly introduced functions is in cl_khr_in64_base_atomics. The specs say:

  The optional extensions cl_khr_int64_base_atomics and cl_khr_int64_extended_atomics
  implement atomic operations on 64-bit signed and unsigned integers to locations in __global
  and __local memory. .
  An application that wants to use any of these extensions will **need** to include the #pragma
  OPENCL EXTENSION cl_khr_int64_base_atomics : enable or #pragma
  OPENCL EXTENSION cl_khr_int64_extended_atomics : enable directive in
  the OpenCL program source

(emphasis mine) The use of pragma is not voluntary, but necessary for the functions to become available.

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

explained above, extensions shouldn't introduce new symbols without being enabled. applications are free to use any symbol names that are not reserved by the specs.
If my application decides to implement `swap_atomically` it's perfectly free to do so and should continue to work even if someone later decides to add `swap_atomically` as extension function.
Requiring such an application to be updated to check for new_extension macro breaks backward compatibility.

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

Namespaces are not necessary. the behaviour is easily implementable using defines:
`#pragma abcdef: enable` introduces `#define exported_function __reserved_name_function`
`#pragma abcdef: disable` removes such definitions.

applications cannot legally use names that begin with two underscores, so there's no risk of name clash

`all identifiers that being with an underscore and a capital letter or another underscore are always reserved for any use` (C99 7.1.3)

  The following names are reserved for use as keywords in OpenCL C and shall not be used
  otherwise.
   * Names reserved as keywords by C99.

Opencl-1.1 6.1.9

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

And it misses the point. legacy application cannot check for macros that didn't exist at the time of writing.
requiring applications to be continually adjusted as new extensions become available breaks backward compatibility.

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

Both cles_khr_int64 and cles_khr_2d_image_array_writes are described in opencl 1.1 and 1.2 specs.
Applications that use opencl 1.2 follow those specs, and clang's std=cl1.2 should follow cl 1.2 specs.
Requiring applications to be continually rewritten as new cl versions become available breaks backward compatibility.

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

The extension is already implemented (char/short dereferences work). it just needs to be exposed to applications performing correct steps to enable extended behaviour.
The same reason why `cl_khr_fp64` is still listed in the extensions list even though it stopped being an extension in cl1.2.

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

The presence of these extensions enables correct functionality of procedures that enable extended behaviour (use of pragmas). Irrespective of whether the same procedure is required in later versions or not.
Correctly written OCL 1.0 applications are required to use legacy pragmas, removing them breaks backward compatibility.

Whether it's implemented as a no-op extension, or just a list of pragma names that shouldn't issue warning or error is an implementation detail.

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

sure. which makes it even more puzzling that you don't want to consider cl1.x specs.

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

This is IMO completely misguided. The policy wrt to applications that follow older spec should be clearly stated and adhered to, giving enough time for any affected users to adapt their workloads in case there are changes.
An ill-defined 'benefit of the community' just means that old applications will get broken unless they actively watch for clang development. at which point it's easier to update legacy apps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372