This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Drop extension pragma handling for extension types/declarations

Authored by Anastasia on Apr 22 2021, 3:40 AM.



The current implementation of extension pragma is not conformant to the spec as it does not disable anything and therefore enabling non-disabled logic has no meaning.

The implementation doesn't respect the following requirement from OpenCL Extension spec s1.2:

disable Behave (including issuing errors and warnings) as if the extension extension_name is not part of the language definition.

This means that extension functionality should not be exposed by default and if extension identifiers are not reserved they should not be recognized by the compiler.

Fixing the behavior doesn't seem easy in C/C++-based parsing as it requires dynamic loading and unloading functionality. In C/C++-based languages, this has never been considered and they provide dedicated language features for loading i.e. include files, namespaces, etc. I don't know languages that actually support such a feature. In GLSL from where the initial idea came from the loading and unloading of extension functionality is constrained to be only available before parsing of the shading sourcing is done.

Considering the severe limitations I would like to drop maintaining this code now especially because of its interference with OpenCL 3.0. If we decide to implement this behavior (although it doesn't seem likely) we should provide the complete fully functional implementation.

The desired logic for exposing extension functionality conditionally can be easily achieved by using the extension macros definition/guards and header files.

Diff Detail

Event Timeline

Anastasia created this revision.Apr 22 2021, 3:40 AM
Anastasia requested review of this revision.Apr 22 2021, 3:40 AM
Anastasia retitled this revision from [OpenCL] Drop extension pragma handling for extension types/declaration to [OpenCL] Drop extension pragma handling for extension types/declarations.Apr 22 2021, 5:26 AM

Did you check whether this patch will cause regression in OpenCL conformance tests? If not, I am OK with it.

Did you check whether this patch will cause regression in OpenCL conformance tests? If not, I am OK with it.

This change has no impact on CTS as CTS has never had negative compiler tests i.e. checking that the compiler rejects certain kernels. This change doesn't break existing kernels i.e. it only allows to compile extra kernels i.e. without the use of pragmas enable/disable when non-native compiler types or functions are called.

CTS only mainly uses #pragma OPENCL EXTENSION * : enable and there is only one occurrence of #pragma OPENCL EXTENSION * : disable that only checks that the pragmas are accepted, there are no other relevant checks in the tests.

yaxunl accepted this revision.Apr 22 2021, 6:30 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Apr 22 2021, 6:30 AM

Reasoning and strategy looks sensible to me. I'm not formally approving the patch because I feel I'm not familiar enough with the code base to do that.

This revision was landed with ongoing or failed builds.May 17 2021, 4:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2021, 4:12 AM
Herald added a subscriber: ldrumm. · View Herald Transcript