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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia 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)
----------------
azabaznov wrote:
> 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.
> 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. 

Good question! I don't think spec ever explained what it means. This is possibly one of multiple interpretations and it won't be easy to change but it would help if spec clarifies it. Potentially we can point out the interpretation from clang implementation. Perhaps it will be taken into account. 


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


================
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");
----------------
azabaznov wrote:
> 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?
> 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?

Yes, we could still simplify the headers by adding the feature macros directly there:

```
#if defined(cl_khr_fp64)
#define __opencl_c_fp64 1
#endif
...
#if defined(__opencl_c_fp64)
double cos(double);
...
#endif
```



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

Ok, this all makes sense. The only question is whether you plan to keep coherency between corresponding features and extensions **automatically** or it has to be done **manually** i.e. targets would be responsible to provide the setup consistently and the same applies to values set in `-cl-ext ` e.g. if it has `+cl_khr_fp64` then it should also have `+__opencl_c_fp64`. The advantage of doing it automatically is that it reduces the risks of errors, but it might become complicated to implement and test all possible combinations.

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

I think right now it only applies to extensions actually. What I mean is if a target sets the extension which is core to unsupported, but I think this was intended to be allowed... it is not explicitly explained in the spec.



================
Comment at: clang/lib/Parse/ParsePragma.cpp:789
     Actions.setCurrentOpenCLExtension("");
   } else if (!Opt.isKnown(Name))
     PP.Diag(NameLoc, diag::warn_pragma_unknown_extension) << Ident;
----------------
azabaznov wrote:
> 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.
Ok, this makes sense since OpenCL 3.0 is experimental in upstream clang so nobody should be using it in production content until it's officially released.


================
Comment at: clang/lib/Sema/Sema.cpp:295
   if (getLangOpts().OpenCL) {
+    getOpenCLOptions().setOpenCLVersion(getLangOpts());
     getOpenCLOptions().addSupport(
----------------
azabaznov wrote:
> 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.
> It may be confusing but now we are using two different OpenCLOptions instances. One in TargetOptions (here) and one in Sema( and here). Not sure why it's not just a reference in Sema...

Yes I agree, having this duplicated seems incorrect. I think one should be a reference indeed.


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

Generally, OpenCL features/extensions are somewhat conceptually in the middle - between LangOpts and target features, mainly because target features are intended for one family of architectures while OpenCL features can be supported by multiple very different devices. However, I agree they might be better implemented using the regular target features. If you are planning to look into it at some point I am interested to discuss your ideas in more details.



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

https://reviews.llvm.org/D89869