[PATCH] D20447: [OpenCL] Fixup extension list

[Yaxun Liu via cfe-commits]

yaxunl added inline comments.

================
Comment at: test/SemaOpenCL/extension-version.cl:11
@@ +10,3 @@
+#endif
+#pragma OPENCL EXTENSION cl_clang_storage_class_specifiers: enable
+
----------------
Anastasia wrote:
> jvesely wrote:
> > Anastasia wrote:
> > > jvesely wrote:
> > > > Anastasia wrote:
> > > > > Could you use standard diagnostic check please:
> > > > >   expected-warning{{unknown OpenCL extension ...
> > > > > 
> > > > > Similarly to SemaOpenCL/extensions.cl
> > > > 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)
> > > 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.
> > 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.
> 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.
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D20447