[PATCH] D89869: [OpenCL] Define OpenCL feature macros for all versions

Thu Nov 26 11:31:38 PST 2020

Anastasia added a comment.

After a recent discussion with Khronos on https://github.com/KhronosGroup/OpenCL-Docs/issues/500 and more thinking I start to realize that there is absolutely nothing new in OpenCL 3.0 features as it isn't a new concept at all. They use to be explained as optional core features in the extension spec from early spec versions. So I think what have changed mainly in OpenCL 3.0 is that the features that were core have now become optional while the current design in clang assumes that core features can't become optional. I feel this is what we should reflect better in the design right now. So the categories we have are:

- extension (conditionally supported, can have pragma to alter certain functionality)
- optional feature (conditionally supported, pragma has no effect)
- core feature (unconditionally supported, pragma has no effect)

Then for every pair of optional functionality and OpenCL language version one of the above categories should be set.

Let's take `cl_khr_3d_image_writes`/`__opencl_c_3d_image_writes` as an example:

- In OpenCL C 1.0-1.2 it is an extension
- In OpenCL C 2.0 it is a core feature
- In OpenCL C 3.0 it is an optional feature

I am now trying to think perhaps the data structure in `OpenCLOptions` should be changed to better match the concept. Then perhaps the flow will become more clear? Right now I find it very complex to understand, mainly because there are features that are being set differently before and after OpenCL 3.0 and then overridden in some places or not set to the expected values. It also doesn't help of course that there are so many existing bugs like with the core features...

I didn't want to end up with a big refactoring from this patch but now I start to think that considering the situation it might be unavoidable. :(

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:42
+
+  // For pre-OpenCL C 3.0 features are supported unconditionally
+  bool isSupportedFeatureForPreOCL30(llvm::StringRef Feat) const {
----------------
I think the comment is misleading a bit because you have a condition on OpenCL version.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:128
+      if (isSupportedFeatureForPreOCL30(Ext))
+        Info.Supported = true;
+    } else
----------------
This is a violation of the API, as its expected to set the feature to a certain value` V`, but here it is forced to be `true`.

Perhaps a better way to handle this is to define whether the feature is core or not and then skip over core features here.

The OpenCL 3.0 features will be defined as core in OpenCL 2.0 but optional in OpenCL 3.0.

Then we can update the API with a more consistent and clear behavior.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:133
+
+  void addSupport(const OpenCLOptions &Opts) {
+    for (auto &I : Opts.OptMap)
----------------
We really need to follow up with some clean up of the duplication and the need to copy entries over.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:37
+  // OpenCL Version
+  unsigned CLVer = 120;
+  bool IsOpenCLCPlusPlus = false;
----------------
Anastasia wrote:
> azabaznov wrote:
> > Anastasia wrote:
> > > Ok, I see why you are adding this field but I am not very happy with it. However, if you prefer to keep it I suggest to add a comment otherwise it is mislediang because ideally in Clang we should be using versions from the LangOpts everywhere. Alternatively we could consider a helper function to calculate the version although it doesn't eliminate the need to the comment.
> > > However, if you prefer to keep it I suggest to add a comment otherwise it is mislediang because ideally in Clang we should be using versions from the LangOpts everywhere
> > 
> > The reason for this is that `LangOptions` is not always available for proper settings. So this just needed internally. Also, we could add `TargetInfo::setOpenCLOptions(const LangOptions &Opts)` where we can set all extensions/features in one step (invoke `::setSupportedOpenCLOpts` and `::setOpenCLExtensionOpts`) to make sure `OpenCLOptions` will get proper OpenCL version. What do you think of that?
> Yes this feels neater!
I feel this just results in more layering violations than we have already made in the past. The version should really be stored in the `LangOpts` and we can either have a reference to it here or/and a helper function to compute the lang version equivalent for the extensions given `LangOpts`.

Perhaps this relates to the fact that `OpenCLOptions` are completely duplicated in Sema... maybe they should have just been there and not in the target settings after all... Or perhaps we are in need for some bigger restructuring in order to avoid making more mess...

================
Comment at: clang/include/clang/Basic/TargetInfo.h:1488
+  /// Set supported OpenCL extensions and optional core features.
+  virtual void setSupportedOpenCLOpts() {}
+  /// Set supported OpenCL extensions as written on command line
----------------
I think the interface is becoming unclear too many members with similar functionality...

================
Comment at: clang/lib/Basic/TargetInfo.cpp:359
   if (Opts.OpenCL) {
+    setOpenCLExtensions(Opts);
+
----------------
Can you explain the reason for this change? I think the meaning of `adjust` function is to override something that was previously set. So I think normal initialization shouldn't really go here.

================
Comment at: clang/lib/Headers/opencl-c-base.h:12

+// Define OpenCL header-only features for pre-OpenCL C 3.0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0)
----------------
for pre-OpenCL -> before

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

https://reviews.llvm.org/D89869