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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
