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

Thu Nov 12 11:18:41 PST 2020

Anastasia added a comment.

In D89869#2388499 <https://reviews.llvm.org/D89869#2388499>, @azabaznov wrote:

> Addressed all concerns except replacing //__opencl_c_int64 //definition into header. The reason for this as follows: this macro should be predefined regardless if clang includes default header or not.

FYI clang doesn't provide full support of OpenCL without the header. In fact, there is ongoing work on making the header included by default without any flag. But I can see that this functionality is related to the frontend and not something that is simply added via libraries so I don't have strong objections for adding it in the clang source code. However, the macro can be added without registering the new extension (see how `__IMAGE_SUPPORT__` is added for example). Right now you are adding a pragma and a target setting that targets can modify without any effect, so I would like to avoid these.

My preference would be if you prepare a separate patch for this. Smaller selfcontained patches are easier and faster to review and also it reduces gaps in testing.

================
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)
----------------
Btw I guess we don't need the last parameter for the features since it's always 0?

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

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:27
     bool Enabled;   // Is this option enabled
+    bool IsFeature; // Is this OpenCL feature
     unsigned Avail; // Option starts to be available in this OpenCL version
----------------
Ok, let's add spec reference where the feature is described so something like:
OpenCL 3.0 s6.2.1

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

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:38
+  unsigned CLVer = 120;
+  bool IsOpenCLCPlusPlus = false;
+
----------------
I don't see a value in such field. We can simply check LangOpts::OpenCLCPlusPlus. 

================
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
----------------
I feel I missed that. Can you explain why it is not the same. Any spec reference would be help.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:49
+  // For pre-OpenCL C 3.0 features are supported simultaneously
+  // with corresponding extensions (if there is such extension, otherwise
+  // check specific version of feature)
----------------
I find this bit  hard to read `(if there is such extension, otherwise check specific version of feature)` how about:

```
Set features for OpenCL C prior to 3.0 based on either:
 - the equivalent extension
 - or language version.
```

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

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:52
+  void supportFeatureForPreOCL30(StringRef Feat, bool On = true) {
+    assert(CLVer < 300 && "Can'be called for OpenCL C higher 3.0");
+    auto It = EquivalentFeaturesExtensions.find(Feat);
----------------
I think the message should be
`Can't be called for OpenCL C 3.0 or higher`

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

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:85
+  bool isSupportedCore(llvm::StringRef Ext) const {
     // In C++ mode all extensions should work at least as in v2.0.
     auto I = OptMap.find(Ext)->getValue();
----------------
I guess this comment should now go to `setOpenCLVersion`. The same for the function below.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:144
+  // specific OpenCL version. For example, OpenCL 2.0 supports
+  // all features that are optional in 3.0
+  void adjustFeatures() {
----------------
This is true apart from subgroups btw.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:145
+  // all features that are optional in 3.0
+  void adjustFeatures() {
+    // Assume compiling for FULL profile
----------------
How about `adjustFeaturesPerOpenCLVersion`

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:151
+      // Simultaneously support feature and equivalent extension.
+      for (auto &FeatExt : EquivalentFeaturesExtensions)
+        OptMap[FeatExt.getValue()].Supported =
----------------
I think it's clearer if this is done while setting the extensions i.e. perhaps we can add a helper `supportExtenions` 

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:155
+
+      // For OpenCL C 3.0 all features are supported
+      // explicitly via option or target settings.
----------------
how about: 
`For OpenCL C 3.0 all features are to be set by the targets explicitly.`

================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:954

+  if (getLangOpts().OpenCL) {
+    getTarget().getSupportedOpenCLOpts().setOpenCLVersion((getLangOpts()));
----------------
I think this block better belongs to `getTarget().adjust(getLangOpts())` too, otherwise we end up doing many similar steps scattered all over.

================
Comment at: clang/lib/Frontend/InitPreprocessor.cpp:1119

     if (TI.getTriple().isSPIR())
       Builder.defineMacro("__IMAGE_SUPPORT__");
----------------
If you would like to add predefined macro for `__opencl_c_int64`,  then this is a good place.

================
Comment at: clang/lib/Parse/ParsePragma.cpp:789
     Actions.setCurrentOpenCLExtension("");
   } else if (!Opt.isKnown(Name))
     PP.Diag(NameLoc, diag::warn_pragma_unknown_extension) << Ident;
----------------
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.

================
Comment at: clang/lib/Sema/Sema.cpp:295
   if (getLangOpts().OpenCL) {
+    getOpenCLOptions().setOpenCLVersion(getLangOpts());
     getOpenCLOptions().addSupport(
----------------
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.

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

https://reviews.llvm.org/D89869