[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.


================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:16
 //
 // If the extensions are to be enumerated without the supported OpenCL version,
 // define OPENCLEXT(ext) where ext is the name of the extension.
----------------
Anastasia wrote:
> yaxunl wrote:
> > Can you add a comment here referring the spec about "Every extension which affects the OpenCL language semantics, syntax or adds built-in functions to the language must create a preprocessor #define that matches the extension name string." and therefore only extensions "which affects the OpenCL language semantics, syntax or adds built-in functions to the language" are defined in this file. Thanks.
> Good idea! I also suggest we add clarifications regarding pragmas, that those are to be added only when the compiler needs to change the compilation flow i.e. if there is a difference in the language semantic from what is defined in a standard.
I've added a comment. Let me know if it suits you.


================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)
----------------
Anastasia wrote:
> azabaznov wrote:
> > mantognini wrote:
> > > azabaznov wrote:
> > > > cl_khr_srgb_image_writes - Extension allowing writes to sRGB images from a kernel. This extension enables write_imagef built-in function as it described [[ https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes | here]]. So I think we shouldn't remove it. Do I miss something?
> > > On second reading, this one is slightly ambiguous. On the language side, the extension doesn't add an overload; it only specifies that existing overload can be used with a different kind of image. On the API side, it does extend the set of features. But at the same time if the extended API is not supported, it's not possible to create an image in the first place and therefore impossible to call write_imagef. So I question the usefulness of such macro on the device side. Does this argument convince you it should be removed?
> > > it's not possible to create an image in the first place and therefore impossible
> > Not quite that right. Referring to [[ https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#conversion-rules-for-srgba-and-sbgra-images | this ]]:
> > 
> > read_imagef built-in functions for OpenCL C 2.0 devices do implicit conversions to RGB for sRGB images. However, implicit conversion from sRGB to RGB is done on write_imagef only if corresponding extension is supported. Otherwise, explicit conversions inside a kernel may be required.
> > 
> > I'm agree that this one is kind of confusing and requires some extra investigation. But I think now we should remove only those extensions which are only API related for sure.
> Ok, thanks for providing extra information. I agree this deserves extra clarifications.
> 
> I am not sure whether the frontend will be able to perform such conversions since it doesn't have information regarding the channels. The image types are completely opaque. That means that potentially the BIFs can handle the conversion internally. However, I am unclear whether the macro can be useful i.e. whether there is anything that can be done differently at the source level based on its availability in OpenCL kernel code.
> 
> It should be ok to leave this out for now from the clean up unless someone else can help quickly with some more insights.
Thanks for the clarification; this indeed requires further investigations so I've removed this part from my patch for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372