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