[PATCH] D96524: [OpenCL] Add support of OpenCL C 3.0 __opencl_c_fp64

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I would prefer if we try to take a slightly different route i.e. if two features are identical (e.g. `cl_khr_fp64` and `__opencl_c_fp64`) we make sure that they are both set identical in Target options or command-line interface using early check and a diagnostic in the driver. This could be done at the end of the frontend options setup. Then for the rest of the compilation including header parsing, we can confidently keep checking only one of them because we know that they are both set consistently and that they are indeed identical. If you stick to checking for `cl_khr_fp64` you can avoid most of the modifications completely.

I have added more details in the code comments. :)



================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:41
+  auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
+  return CLVer >= 300 ? isAvailableOption("__opencl_c_fp64", LO)
+                      : isAvailableOption("cl_khr_fp64", LO);
----------------
btw since `cl_khr_fp64` is available in all versions could we not just check only for it?


================
Comment at: clang/lib/Basic/TargetInfo.cpp:398
+    auto CLVer = Opts.OpenCLCPlusPlus ? 200 : Opts.OpenCLVersion;
+    if (CLVer >= 300) {
+      auto &OCLOpts = getSupportedOpenCLOpts();
----------------
azabaznov wrote:
> Anastasia wrote:
> > azabaznov wrote:
> > > Anastasia wrote:
> > > > I suggest we move this onto `OpenCLOptions::addSupport`.
> > > This should stay here to control simultaneous macro definitions
> > Could we leave this bit out? These are set correctly by the targets already... and I think targets do need to set those explicitly indeed. I don't see big value in this functionality right now.
> There are 2 reasons why it should be here
> 
> 1) Simultaneous macro definition
> 
> 2) Correct option processing: we need to remove `cl_khr_fp64` macro if `-cl-ext=-__opencl_c_fp64` specified
Ok as I said I would prefer to stick to an explicit interface though, i.e. every feature should be set explicitly
`-cl-ext=-__opencl_c_fp64,-cl_khr_fp64`
as the interface gets too messy otherwise...

I believe we won't have that many of those case - is it just fp64, fp16 and images?

If you are worried about the inconsistent uses (which is a valid concern!) how about we just add a check with a diagnostic in clang driver about the consistency of the setup between the language, command-line options and the target. We already discussed such check in other use cases - to help avoid using incorrect language versions that the target has no support. 

I think this route will be easier than keeping consistency automatically and silently overriding the options i.e. we already had bad experiences with bugs due to such logic. A diagnostic would be very helpful both to application developers and tooling developers and help to avoid extra complexity in clang too.


================
Comment at: clang/lib/Headers/opencl-c-base.h:530
 
-#ifdef cl_khr_fp64
+#if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
 #define as_double(x) __builtin_astype((x), double)
----------------
Let's only use one macro as they signal the same. I don't mind which one you choose perhaps `__opencl_c_fp64` could be used since we can freely define it in the earliest versions or we can continue using `cl_khr_fp64` then no changes are needed other then making sure it is defined when `__opencl_c_fp64` is defined which can be done at the top of this header btw.


================
Comment at: clang/lib/Headers/opencl-c.h:4635
 
-#ifdef cl_khr_fp64
+#if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
 #pragma OPENCL EXTENSION cl_khr_fp64 : enable
----------------
Anastasia wrote:
> azabaznov wrote:
> > azabaznov wrote:
> > > Anastasia wrote:
> > > > svenvh wrote:
> > > > > Wondering if we need a similar change in `clang/lib/Headers/opencl-c-base.h` to guard the double<N> vector types?
> > > > Instead of growing the guard condition, I suggest we only check for one for example the feature macro and then make sure the feature macro is defined in `opencl-c-base.h` if the extensions that aliases to the feature is supported. However, it would also be ok to do the opposite since the extensions and the corresponding features should both be available.
> > > > 
> > > > FYI, something similar was done in https://reviews.llvm.org/D92004.
> > > > 
> > > > This will also help to propagate the functionality into Tablegen header because we won't need to extend it to work with multiple extensions but we might still need to do the rename.
> > > Yeah, I overlooked this place... Thanks!
> > I don't think that growing of this condition will hurt in some cases... Note, that unifying condition check makes sense if some features are unconditionally supported for OpenCL C 2.0, such as generic address space for example. In other cases (such as fp64, 3d image writes, subgroups) we should use OR in guard condition.  
> > 
> > Your approach will require extra checks in clang such as, for example, to make sure that extension macro is not predefined if the feature macro is defined, because there will be redefinition since extension macro is defined in header.
> I think using one macro for everything is just simpler and cleaner.
> 
> > Your approach will require extra checks in clang such as, for example, to make sure that extension macro is not predefined if the feature macro is defined, because there will be redefinition since extension macro is defined in header.
> 
> I think we should handle this in the headers instead and we should definitely define the macros conditionally to avoid redefinitions.
Do you still plan to address this? FYI this comment is the same as in `opencl-c-base.h`.


================
Comment at: clang/lib/Sema/Sema.cpp:305
+    llvm::StringRef FP64Feature(
+        getLangOpts().OpenCLVersion >= 300 ? "__opencl_c_fp64" : "cl_khr_fp64");
     if (getLangOpts().OpenCLCPlusPlus || getLangOpts().OpenCLVersion >= 200) {
----------------
But `cl_khr_fp64` is also available if `__opencl_c_fp64` is available. If anything it should be even more available for backward compatibility. I don't see the point in checking the same thing twice or checking it differently.

If you are worried you could add some comment/documentation to clarify that they both have to be set or unset simultaneously and a check in the Clang driver initialization verifying the consistency in the settings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96524