Anastasia added a comment.
After a recent discussion with Khronos on
https://github.com/KhronosGroup/OpenCL-Docs/issues/500 and more thinking I
start to realize that there is absolutely nothing new in OpenCL 3.0 features as
it isn't a new concept at all. They use to be explained as optional core
features in the extension spec from early spec versions. So I think what have
changed mainly in OpenCL 3.0 is that the features that were core have now
become optional while the current design in clang assumes that core features
can't become optional. I feel this is what we should reflect better in the
design right now. So the categories we have are:
- extension (conditionally supported, can have pragma to alter certain
functionality)
- optional feature (conditionally supported, pragma has no effect)
- core feature (unconditionally supported, pragma has no effect)
Then for every pair of optional functionality and OpenCL language version one
of the above categories should be set.
Let's take `cl_khr_3d_image_writes`/`__opencl_c_3d_image_writes` as an example:
- In OpenCL C 1.0-1.2 it is an extension
- In OpenCL C 2.0 it is a core feature
- In OpenCL C 3.0 it is an optional feature
I am now trying to think perhaps the data structure in `OpenCLOptions` should
be changed to better match the concept. Then perhaps the flow will become more
clear? Right now I find it very complex to understand, mainly because there are
features that are being set differently before and after OpenCL 3.0 and then
overridden in some places or not set to the expected values. It also doesn't
help of course that there are so many existing bugs like with the core
features...
I didn't want to end up with a big refactoring from this patch but now I start
to think that considering the situation it might be unavoidable. :(
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:42
+
+ // For pre-OpenCL C 3.0 features are supported unconditionally
+ bool isSupportedFeatureForPreOCL30(llvm::StringRef Feat) const {
----------------
I think the comment is misleading a bit because you have a condition on OpenCL
version.
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:128
+ if (isSupportedFeatureForPreOCL30(Ext))
+ Info.Supported = true;
+ } else
----------------
This is a violation of the API, as its expected to set the feature to a certain
value` V`, but here it is forced to be `true`.
Perhaps a better way to handle this is to define whether the feature is core or
not and then skip over core features here.
The OpenCL 3.0 features will be defined as core in OpenCL 2.0 but optional in
OpenCL 3.0.
Then we can update the API with a more consistent and clear behavior.
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:133
+
+ void addSupport(const OpenCLOptions &Opts) {
+ for (auto &I : Opts.OptMap)
----------------
We really need to follow up with some clean up of the duplication and the need
to copy entries over.
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:37
+ // OpenCL Version
+ unsigned CLVer = 120;
+ bool IsOpenCLCPlusPlus = false;
----------------
Anastasia wrote:
> 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!
I feel this just results in more layering violations than we have already made
in the past. The version should really be stored in the `LangOpts` and we can
either have a reference to it here or/and a helper function to compute the lang
version equivalent for the extensions given `LangOpts`.
Perhaps this relates to the fact that `OpenCLOptions` are completely duplicated
in Sema... maybe they should have just been there and not in the target
settings after all... Or perhaps we are in need for some bigger restructuring
in order to avoid making more mess...
================
Comment at: clang/include/clang/Basic/TargetInfo.h:1488
+ /// Set supported OpenCL extensions and optional core features.
+ virtual void setSupportedOpenCLOpts() {}
+ /// Set supported OpenCL extensions as written on command line
----------------
I think the interface is becoming unclear too many members with similar
functionality...
================
Comment at: clang/lib/Basic/TargetInfo.cpp:359
if (Opts.OpenCL) {
+ setOpenCLExtensions(Opts);
+
----------------
Can you explain the reason for this change? I think the meaning of `adjust`
function is to override something that was previously set. So I think normal
initialization shouldn't really go here.
================
Comment at: clang/lib/Headers/opencl-c-base.h:12
+// Define OpenCL header-only features for pre-OpenCL C 3.0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0)
----------------
for pre-OpenCL -> before
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
