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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 19 05:17:54 PST 2020


Anastasia added inline comments.


================
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:
> > 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.
> > 
> > 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 think it's possible to handle these in our `OpenCLOptions`, e.g. automatically. This will release targets from obligations to take of this simultaneous setting: each target will need to duplicate this (introduce helper...?).  For now I think it's possible to reuse `setSupportedOpenCLOpt` for extensions and features.
> 
> > 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
> 
> This shouldn't affect anything if feature unsupported. Just to double check, you mean target calls:
> 
> ```
> support("-cl_khr_fp64");
> support("+__opencl_c_fp64");
> ```
> 
> As I see it compiler should define both macros both for OpenCL C 3.0 and nothing for fp64 in earlier versions.
> 
> P.S. actually I didn't find something similar about fp64 as for [[ https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_API.html#_3d_image_writes| 3d images ]] in API reports. I'm wondering if spec is missing that for fp64, or is something similar to subgroups, and extension/feature are not the same....
> 
> 
> P.S. actually I didn't find something similar about fp64 as for 3d images in API reports. I'm wondering if spec is missing that for fp64, or is something similar to subgroups, and extension/feature are not the same....

Yes, potentially we need to brush up the spec a bit more for extensions and features that are inherited from extensions. I find the spec very inconsistent. In some cases it is not even clear what extensions even mean. Here is one example: https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_depth_images.html

Not sure what's the best approach as it's a lot of work. Potentially we should create spec issues as we go along with the implementation. I think perhaps we should expand section 6.2.2. to at least explain what the optional core extension actually means?






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

https://reviews.llvm.org/D89869



More information about the cfe-commits mailing list