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