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

Mon Feb 15 01:18:59 PST 2021

azabaznov added inline comments.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:157
+  // Is OpenCL C feature (OpenCL C 3.0, 6.2.1. Features)
+  bool isFeature(llvm::StringRef Ext) const;
+
----------------
svenvh wrote:
> The argument "Ext" suggests "Extension", so perhaps rename if this is about features? (also in the definition in the .cpp file)
Sure, thanks. I was going to rename 'extensions' to other concept which will represent extensions and features, but this will be done in a separate  commit

================
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");
+}
----------------
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.

================
Comment at: clang/lib/Basic/TargetInfo.cpp:395
+
+    // Set extensions simultaneosly with correspoding features
+    // for OpenCL C 3.0 and higher
----------------
svenvh wrote:
> simultaneosly -> simultaneously
> correspoding -> corresponding
Sure, thanks

================
Comment at: clang/lib/Basic/TargetInfo.cpp:398
+    auto CLVer = Opts.OpenCLCPlusPlus ? 200 : Opts.OpenCLVersion;
+    if (CLVer >= 300) {
+      auto &OCLOpts = getSupportedOpenCLOpts();
----------------
Anastasia wrote:
> I suggest we move this onto `OpenCLOptions::addSupport`.
This should stay here to control simultaneous macro definitions

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

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

================
Comment at: clang/lib/Sema/Sema.cpp:356
+      if (getLangOpts().OpenCLVersion >= 300)
+        setOpenCLExtensionForType(AtomicDoubleT, "__opencl_c_fp64");
+      else
----------------
Anastasia wrote:
> I think we should just guard
> 
> ```
> addImplicitTypedef("atomic_double", AtomicDoubleT);
> ```
> 
> by the feature support instead...
> 
> Or alternatively, we could also just change this entire diagnostic set up to check for extensions and features being supported and not enabled.
> 
> Frankly, I would prefer the first solution because this aligns with C/C++ concept. It is not possible for the compiler to know all possible types and functions that are added via the libraries so this diagnostic only worked for some cases (that are added to the compiler) but not all cases. However, I don't think we need to expose to the developer where and how features are implemented. This will avoid unnessasary questions - why in some cases they get one diagnostic and in the other case they get another diagnostic. It will allow us to change the implementation without the developers to notice. And considering that this doesn't break backward compatibility it is a pretty reasonable direction in my opinion.
> I think we should just guard

Hmm, I've never though of it but is sounds like a good idea. I'll double-check that and see how it will work!

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96524