[OpenCL] Fixup extension list

[Anastasia]

It seems like there is no test covering this?

Could you add a test in SemaOpenCL checking this code i.e. extension is allowed in CL>1.2 and disallowed if CL<1.2?

Thanks!

[Anastasia]

Please add cfe-commits in subscriber list!

[jvesely]

I went through the specs and fixed up all I could find.
added test

[Anastasia]

test/SemaOpenCL/extension-version.cl

11 ↗

(On Diff #58130)

Could you use standard diagnostic check please:

expected-warning{{unknown OpenCL extension ...

Similarly to SemaOpenCL/extensions.cl

[jvesely]

test/SemaOpenCL/extension-version.cl

11 ↗

(On Diff #58130)

not sure I follow, the test does not trigger any diagnostics (by design).
are you saying that I should introduce negative checks to make sure extensions are not available outside of their respective context?
Is there a way to filter verifier tags based on clang invocation? (something like FileCheck prefix)

[Anastasia]

test/SemaOpenCL/extension-version.cl

11 ↗

(On Diff #58130)

Exactly, you should check that the extensions are enabled correctly based on CL versions.

For example if you compile this without passing -cl-std=CL1.2:

#pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing: enable

the following error is produced:

unsupported OpenCL extension 'cl_khr_gl_msaa_sharing' - ignoring

You can condition error directives on CL version passed as it's done in the example test SemaOpenCL/extensions.cl.

So what is the original intension of this tests? Not sure I understand what you are trying to test.

[jvesely]

test/SemaOpenCL/extension-version.cl

11 ↗

(On Diff #58130)

it's a positive test that checks that extensions are available (both that the define is present, and that #pragma passes without error).

I did not include negative tests (check that extension is not available outside of its respective context), because I think it's a bit overzealous reading of the specs.
For example cl_khr_d3d10_sharing is first mentioned in OpenCL 1.1 specs, but the text of the extension says that it is written against OpenCL 1.0.48 spec. (I moved cl_khr_icd to 1.0 for the same reason). I think if a vendor can add vendor specific extensions to the list of supported extensions, it should be possible to add extensions from higher CL versions.

similarly, I would argue against warnings for extensions promoted to core features (or at least hide the warning behind -pedantic). they are listed in CL_DEVICE_EXTENSIONS for backwards compatibility so I'd say it is OK to allow pragmas in higher CLC versions for backward compatibility.

[Anastasia]

test/SemaOpenCL/extension-version.cl

11 ↗

(On Diff #58130)

I agree with this:

"similarly, I would argue against warnings for extensions promoted to core features (or at least hide the warning behind -pedantic). they are listed in CL_DEVICE_EXTENSIONS for backwards compatibility so I'd say it is OK to allow pragmas in higher CLC versions for backward compatibility."

@yaxunl, what's your opinion here?

Regarding the test, I think we should still check the diagnostics being given correctly especially for the extensions unavailable in the earlier versions. It should be quite straight forward to extend this test.

[yaxunl]

test/SemaOpenCL/extension-version.cl

11 ↗

(On Diff #58130)

The warning is a reminder that this is no longer an extension and the user should remove that. However I do not have strong opinion on that.

[jvesely]

add detection of extensions in early CL versions to test.

[arsenm]

test/SemaOpenCL/extension-version.cl

12 ↗

(On Diff #58806)

When the AMD compiler added these warnings a long time ago I found them obnoxious and required adding more complicated ifdef combinations to silence them

[jvesely]

test/SemaOpenCL/extension-version.cl

12 ↗

(On Diff #58806)

moved to D20744

[Anastasia]

test/SemaOpenCL/extension-version.cl

41 ↗

(On Diff #58806)

COre -> core

73 ↗

(On Diff #58806)

Could you put a comment to make it more readable, something like

#endif // (OPENCL_C_VERSION < 110)

[Anastasia]

test/SemaOpenCL/extension-version.cl

41 ↗

(On Diff #58806)

Actually looking at other comments -> Core

[jvesely]

test/SemaOpenCL/extension-version.cl

73 ↗

(On Diff #58806)

These large blocks get broken into ~7 line pieces, in D20744. Do you still want the endif comments there, or should I add the comments here and remove them in D20744?

[Anastasia]

test/SemaOpenCL/extension-version.cl

73 ↗

(On Diff #58806)

Ok, no problem. Let's just leave it as is. I guess the other change will be committed soon. :)

[jvesely]

Fix typo: COre -> Core

[Anastasia]

LGTM! Thanks!

include/clang/Basic/OpenCLExtensions.def

 OPENCLEXT_INTERNAL(cl_khr_int64_base_atomics, 110, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_int64_extended_atomics, 110, ~0U)
 
 // OpenCL 1.2.
 OPENCLEXT_INTERNAL(cl_khr_d3d11_sharing, 120, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_depth_images, 120, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_dx9_media_sharing, 120, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_gl_depth_images, 120, ~0U)
+OPENCLEXT_INTERNAL(cl_khr_gl_msaa_sharing, 120, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_spir, 120, ~0U)
 
 // OpenCL 2.0.
 OPENCLEXT_INTERNAL(cl_khr_egl_event, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_egl_image, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_gl_msaa_sharing, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_initialize_memory, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_terminate_context, 200, ~0U)
 
 // Clang Extensions.
 OPENCLEXT_INTERNAL(cl_clang_storage_class_specifiers, 100, ~0U)
 
 #undef OPENCLEXT_INTERNAL
 
 #ifdef OPENCLEXT
 #undef OPENCLEXT
 #endif