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