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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
