[PATCH] D71460: [OpenCL] Fix support for cl_khr_mipmap_image_writes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks!



================
Comment at: clang/lib/Headers/opencl-c.h:14686
+#if defined(cl_khr_mipmap_image_writes)
+#pragma OPENCL EXTENSION cl_khr_mipmap_image_writes : begin
 void __ovld write_imagef(write_only image1d_t image, int coord, int lod, float4 color);
----------------
AlexeySotkin wrote:
> Anastasia wrote:
> > AlexeySotkin wrote:
> > > Anastasia wrote:
> > > > Do we actually need pragma for this extension? I.e. does it need to activate any special mode in the compiler?
> > > I'm not aware of any special mode required for this extension. These begin/end pragma lines were added to disable the extension by default as it is [[ https://github.com/KhronosGroup/OpenCL-Docs/blob/master/ext/introduction.asciidoc#compiler-directives-for-optional-extensions | required ]] by the OpenCL extension spec. Is there any other mechanism which should be used for this purpose?
> > > Probably, we should do the same for `cl_khr_mipmap_image`(and maybe others?), because with the current compiler, built-ins from this extension can be compiled successfully even if `#pragma OPENCL EXTENSION cl_khr_mipmap_image : disable` is specified. See https://godbolt.org/z/fNEWuG
> > What I am saying is that `pragma` is only typically used to activate some special mode in the compiler. If we don't need to activate anything perhaps we don't need to add `pragma` at all? It simplifies compiler and application code too. Would regular include mechanisms be enough to guard the availability of those functions?
> > 
> > Maybe this thread will help to understand the topic better: https://github.com/KhronosGroup/OpenCL-Docs/issues/82
> I think you are right. I have removed the pragma.
I think we probably should get rid of most of extensions in clang/include/clang/Basic/OpenCLExtensions.def and just try to implement the extensions macro using target specific header.

But it doesn't belong to your change and it's probably too big of the change anyway. Just a thought. :)


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

https://reviews.llvm.org/D71460