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

Mon Feb 15 05:35:09 PST 2021

Anastasia added inline comments.

================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:24
+  auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
+  return CLVer >= 300 ? isEnabled("__opencl_c_fp64") : isEnabled("cl_khr_fp64");
+}
----------------
azabaznov wrote:
> Anastasia wrote:
> > We should at least be checking for `isSupported("__opencl_c_fp64")`, but frankly I would prefer to check for supported and not for enabled for `cl_khr_fp64` too. Note that we don't break backward compatibility if we change this because the existing kernels will still be valid and it makes things easier for writing new kernels too.
> I think everything fine with this for now because `OpenCLOptions::enableSupportedCore` is called to set all supported core or optional core features to enabled state. So you suggest to removing this method at all too?
> 
> I think with your approach things will be unobvious in code: for some extensions there will be check for `isSupported` for some other there will be check for `isEnabled`. I think we should stay consistent here and check availability of all options in the same manner.
> I think everything fine with this for now because OpenCLOptions::enableSupportedCore is called to set all supported core or optional core features to enabled state. So you suggest to removing this method at all too?

Yes, I find it redundant somehow. Maybe it's best to indeed remove enabling functionality for features since we definitely don't plan to use pragmas for those? However, I appreciate it might be better to do as a separate change.

> I think with your approach things will be unobvious in code: for some extensions there will be check for isSupported for some other there will be check for isEnabled. I think we should stay consistent here and check availability of all options in the same manner.

That's right, we might get some inconsistency at the start. But I think we should drive towards checking `isSupported` rather than `isEnabled`. I don't think we will have many cases for `isEnabled` at the end.

================
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:
> > 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.

================
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
----------------
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.

================
Comment at: clang/test/SemaOpenCL/opencl-fp64-feature.cl:1
+// Test with a target not supporting fp64.
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -DNOFP64
----------------
azabaznov wrote:
> Anastasia wrote:
> > I suggest we merge this with `extensions.cl` that has similar functionality. As it mainly tests doubles I don't mind if you prefer to rename it then.
> Sure, I'll try that. At first, I just didn't want to modify test's  guard checks as it will become messy:
> 
> ```
> #if __OPENCL_C_VERSION__ >= 300
> // expected-error at -2{{use of type 'double' requires __opencl_c_fp64 feature to be enabled}}
> #else
> // expected-error at -4{{use of type 'double' requires cl_khr_fp64 feature to be enabled}}
> #endif
> ```
> 
> I see if I can avoid that. Maybe also try to unify the diagnostic for that case?
I agree, perhaps regular expressions in the diagnostics could help too i.e. `expected-error-re`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96524