This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Fixup extension list
ClosedPublic

Authored by jvesely on May 19 2016, 12:56 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jvesely updated this revision to Diff 57843.May 19 2016, 12:56 PM
jvesely retitled this revision from to [OpenCL] cl_khr_msaa_sharing is OpenCL1.2 extension.
jvesely updated this object.
jvesely added a reviewer: Anastasia.
jvesely set the repository for this revision to rL LLVM.
jvesely added a subscriber: yaxunl.
Anastasia edited edge metadata.May 23 2016, 9:22 AM

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!

Please add cfe-commits in subscriber list!

jvesely updated this revision to Diff 58130.May 23 2016, 12:11 PM
jvesely retitled this revision from [OpenCL] cl_khr_msaa_sharing is OpenCL1.2 extension to [OpenCL] Fixup extension list.
jvesely edited edge metadata.
jvesely added a subscriber: cfe-commits.

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

Anastasia added inline comments.May 24 2016, 7:40 AM
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 added inline comments.May 24 2016, 10:24 AM
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 added inline comments.May 24 2016, 11:32 AM
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 added inline comments.May 24 2016, 12:38 PM
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 added inline comments.May 25 2016, 7:56 AM
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 added inline comments.May 27 2016, 8:07 AM
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 updated this revision to Diff 58806.May 27 2016, 10:35 AM

add detection of extensions in early CL versions to test.

arsenm added a subscriber: arsenm.May 27 2016, 10:56 AM
arsenm added inline comments.
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 marked 7 inline comments as done.
jvesely added inline comments.
test/SemaOpenCL/extension-version.cl
12 ↗(On Diff #58806)

moved to D20744

Anastasia added inline comments.Jun 1 2016, 8:02 AM
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 added inline comments.Jun 1 2016, 8:04 AM
test/SemaOpenCL/extension-version.cl
41 ↗(On Diff #58806)

Actually looking at other comments -> Core

jvesely marked 2 inline comments as done.Jun 1 2016, 9:28 AM
jvesely added inline comments.
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 added inline comments.Jun 1 2016, 9:37 AM
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 updated this revision to Diff 59240.Jun 1 2016, 9:42 AM

Fix typo: COre -> Core

Anastasia accepted this revision.Jun 1 2016, 9:53 AM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision is now accepted and ready to land.Jun 1 2016, 9:53 AM
This revision was automatically updated to reflect the committed changes.
jvesely marked 5 inline comments as done.