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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9899
 def err_opencl_requires_extension : Error<
-  "use of %select{type|declaration}0 %1 requires %2 extension to be enabled">;
+  "use of %select{type|declaration}0 %1 requires %2 %select{extension|feature}3 to be enabled">;
 def warn_opencl_generic_address_space_arg : Warning<
----------------
We shouldn't require enabling the features besides features and extensions are conceptually the same, let's avoid adding too much complexity due to the unfortunate differences in the spec wording. 

I suggest changing this to something like:

```
use of %select{type|declaration}0 %1 requires %2 support
```


FYI I would also be happy if we don't guard the types by the pragma at all and just allow using the type freely if the extensions is supported. This already happens for the functions so it is not clear why functions and types should be different. The benefit of the pragma guarding hasn't been found after more than a year of investigations.


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


================
Comment at: clang/lib/Basic/TargetInfo.cpp:398
+    auto CLVer = Opts.OpenCLCPlusPlus ? 200 : Opts.OpenCLVersion;
+    if (CLVer >= 300) {
+      auto &OCLOpts = getSupportedOpenCLOpts();
----------------
I suggest we move this onto `OpenCLOptions::addSupport`.


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


================
Comment at: clang/lib/Sema/Sema.cpp:356
+      if (getLangOpts().OpenCLVersion >= 300)
+        setOpenCLExtensionForType(AtomicDoubleT, "__opencl_c_fp64");
+      else
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96524