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

Tue Nov 17 02:39:54 PST 2020

azabaznov added inline comments.

================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:100
+// OpenCL features
+OPENCLFEAT_INTERNAL(__opencl_c_pipes, 200, ~0U)
+OPENCLFEAT_INTERNAL(__opencl_c_generic_address_space, 200, ~0U)
----------------
Anastasia wrote:
> Btw I guess we don't need the last parameter for the features since it's always 0?
This need to be restructured at all, now I just wanted to be consistent. I think there was wrong interpretation of "core extension" concept. //Core //means nothing more than it was just promoted to core specification, not supported by default starting from specific OpenCL version. Am I missing something in the spec? However, I'm not sure if we need to reimplement this to maintain backward compatibility... Anyway, I'll try to remove it from features.

================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:109
+OPENCLFEAT_INTERNAL(__opencl_c_program_scope_global_variables, 200, ~0U)
+OPENCLFEAT_INTERNAL(__opencl_c_fp64, 120, ~0U)
+OPENCLFEAT_INTERNAL(__opencl_c_images, 100, ~0U)
----------------
Anastasia wrote:
> I am thinking maybe we could add an extra parameter where we can specify the extension it is aliased to:
> 
> ```
> OPENCLFEAT_INTERNAL(__opencl_c_fp64, 120, ~0U, cl_khr_fp64)
> ```
> 
> Then it makes clearer which features correspond to which extensions and you can populate `EquivalentFeaturesExtensions` from this field? Moreover, you can then even make a map of references to `OpenCLOptions::Info` so you don't need to look them up from the name every time.
> 
> 
> The drawback is that we need an extra parameter that is mainly empty... however we could recycle the last parameter that is always 0 right now.
> 
> OPENCLFEAT_INTERNAL(__opencl_c_fp64, 120, ~0U, cl_khr_fp64)
Yes, that's sound reasonable to me

> The drawback is that we need an extra parameter that is mainly empty
I don't think we need to keep it `OpenCLOptions::Info`, we can always this from `OpenCLOptions`. 

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:37
+  // OpenCL Version
+  unsigned CLVer = 120;
+  bool IsOpenCLCPlusPlus = false;
----------------
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?

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:42
+
+  // Note that __opencl_c_subgroups and cl_khr_subgroups are not equivalent
+  // because extension requires sub-group independent forward progress
----------------
Anastasia wrote:
> I feel I missed that. Can you explain why it is not the same. Any spec reference would be help.
Ah, yeah. This one is confusing.

You can check [[ https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_API.html#platform-querying-devices| 4.2. Querying Devices ]], the CL_DEVICE_SUB_GROUP_INDEPENDENT_FORWARD_PROGRESS entry:

//This query must return CL_TRUE for devices that support the cl_khr_subgroups extension, and must return CL_FALSE for devices that do not support subgroup//.

It was missing before OpenCL C 2.1 and subgroup independent forward progress is optional in 2.1 and 3.0. Agree that spec is not clear here though. This is just the case when feature describes more functionality than the extension.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:51
+  // check specific version of feature)
+  void supportFeatureForPreOCL30(StringRef Feat, bool On = true) {
+    assert(CLVer < 300 && "Can'be called for OpenCL C higher 3.0");
----------------
Anastasia wrote:
> I find the name of this function very confusing but I can't think of any better one. Also the flow is becoming harder to understand to be honest. This function is not as straight forward as `support` because it might not actually do what is expected from it. I wonder if the name with something like `adjust` would be more appropriate. At the same time `adjustFeatures` is the only place where we need to check for the version. Perhaps you can just do the language version check straight in `adjustFeatures`?
> 
> Overall, let's just get clear about the flow of setting the features and extensions. If in `adjustFeatures` we set default features supported in a certain language version then targets can set the other features. Now let's examine what should happen with the features and extensions on the following use cases:
> - Do we always set the equivalent extension when the feature is being set by the target?
> - Do we always set the equivalent feature when the extension is being set by the target?
> - What happens when equivalent features and extensions are set but to different values?
> - What if targets set core feature/extension to unsupported?
> - How does `-cl-ext` modify core features/extensions and equivalent features+extensions?
> 
> I am a bit worried about the fact that we have different items for the same optional functionality in `OpenCLOptions` as it might be a nightmare to keep them consistent. We can however also choose a path of not keeping them consistent in the common code and rely on targets to set them up correctly.
> 
> Initially, when we discussed adding feature macros to earlier standards I was thinking of simplifying the design. For example instead of checking for extension macros and feature macros in the header when declaring certain functions we could only check for one of those. The question whether we can really achieve the simplifications and at what cost.
Ok.

Now I think that defining features for pre-OpenCL C 3.0 if they have equivalent extension indeed brings up some complexities. The check for the support of features in header will still look like follows:

```
#if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
...
#endif
```

so there is no need to add a feature macro for pre-OpenCL C 3.0 if there is a same extension to check. Are you agree with that?

However, I still see a reason for defining  equivalent extension for OpenCL C 3.0 if feature is supported (backward compatibility with earlier OpenCL C kernels).

> Overall, let's just get clear about the flow of setting the features and extensions
IMO the main thing which we need to try to follow here is that features are stronger concept than extensions in OpenCL C 3.0. The reason for this is that API won't report extension support if the feature is not supported ([[ https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_API.html#_3d_image_writes | 3d image writes example]]. To be clear, if users need to support functionality in OpenCL C 3.0 they should use features, for earlier versions they should use extensions.

Answering your questions:

> Do we always set the equivalent extension when the feature is being set by the target?
I think we should do it for OpenCL C 3.0 only to maintain backward compatibility with OpenCL C 1.2 applications.

> Do we always set the equivalent feature when the extension is being set by the target
I think we shouldn't set equivalent feature for pre-OpenCL C 3.0 since there is no need in that. This brings up some complexities, and we can check an extension presence.

> What happens when equivalent features and extensions are set but to different values
We need to avoid that case for OpenCL C 3.0 since implementation could not report extension support if equivalent feature is not supported.

> How does -cl-ext modify core features/extensions and equivalent features+extensions
'-сl-ext' should not affect feature support in pre-OpenCL 3.0. In OpenCL C 3.0 it's more likely to use features instead of extensions since it's a stronger concept; and equivalent feature+extension will be supported simultaneously if feature is turned on.

And a question:

> What if targets set core feature/extension to unsupported?
Not sure what do you mean about //core// here. What do you mean? But I don't think this will cause a problem if we will follow up the rules that I described above. Do you see any?

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:55
+    if (It != EquivalentFeaturesExtensions.end())
+      OptMap[Feat].Supported = OptMap[It->getValue()].Supported;
+    else if (OptMap[Feat].Avail <= CLVer)
----------------
Anastasia wrote:
> If `On` is `true` but the extension is unsupported then the feature will also stay unsupported despite of the value of `On`. This might be a bit surprising though.
Ah, yeah... I'll double check that and will try to avoid that case. Thanks.

================
Comment at: clang/lib/Parse/ParsePragma.cpp:789
     Actions.setCurrentOpenCLExtension("");
   } else if (!Opt.isKnown(Name))
     PP.Diag(NameLoc, diag::warn_pragma_unknown_extension) << Ident;
----------------
Anastasia wrote:
> I think here you should check for whether it is a feature and if so the pragma is ignored with a warning.
> 
> We probably should add a test for pragmas too.
Sure, this is part of my plans. I just wanted to separate into another patch because features are not handled in clang at all for now.

================
Comment at: clang/lib/Sema/Sema.cpp:295
   if (getLangOpts().OpenCL) {
+    getOpenCLOptions().setOpenCLVersion(getLangOpts());
     getOpenCLOptions().addSupport(
----------------
Anastasia wrote:
> I think the version here should already have been set in `CompilerInstance.cpp`? Perhaps we could even set it in the constructor of `OpenCLOptions`, otherwise, there is a little value in setting this field if we end up calling this helper multiple times instead of passing LangOpts into the original member function.
It may be confusing but now we are using two different `OpenCLOptions` instances. One in `TargetOptions` ([[ https://reviews.llvm.org/source/llvm-github/browse/master/clang/include/clang/Basic/TargetOptions.h$65 | here]]) and one in `Sema`( [[ https://reviews.llvm.org/source/llvm-github/browse/master/clang/include/clang/Sema/Sema.h$394| and here ]]). Not sure why it's not just a reference in `Sema`... Anyway, passing `LangOptions` into ctor is not possible at least for now. I would prefer to keep a reference to `OpenCLOptions `in `Sema`. I was also thinking about unifying with target features ([[ https://reviews.llvm.org/source/llvm-github/browse/master/clang/lib/Basic/Targets.cpp$687| like here]]) and handle OpenCL features/extensions in some similar manners. But I think this a long term one and relates more to a refactoring.

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

https://reviews.llvm.org/D89869