Anastasia added inline comments.
================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:24
+ auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
+ return CLVer >= 300 ? isEnabled("__opencl_c_fp64") :
isEnabled("cl_khr_fp64");
+}
----------------
azabaznov wrote:
> Anastasia wrote:
> > We should at least be checking for `isSupported("__opencl_c_fp64")`, but
> > frankly I would prefer to check for supported and not for enabled for
> > `cl_khr_fp64` too. Note that we don't break backward compatibility if we
> > change this because the existing kernels will still be valid and it makes
> > things easier for writing new kernels too.
> I think everything fine with this for now because
> `OpenCLOptions::enableSupportedCore` is called to set all supported core or
> optional core features to enabled state. So you suggest to removing this
> method at all too?
>
> I think with your approach things will be unobvious in code: for some
> extensions there will be check for `isSupported` for some other there will be
> check for `isEnabled`. I think we should stay consistent here and check
> availability of all options in the same manner.
> I think everything fine with this for now because
> OpenCLOptions::enableSupportedCore is called to set all supported core or
> optional core features to enabled state. So you suggest to removing this
> method at all too?
Yes, I find it redundant somehow. Maybe it's best to indeed remove enabling
functionality for features since we definitely don't plan to use pragmas for
those? However, I appreciate it might be better to do as a separate change.
> I think with your approach things will be unobvious in code: for some
> extensions there will be check for isSupported for some other there will be
> check for isEnabled. I think we should stay consistent here and check
> availability of all options in the same manner.
That's right, we might get some inconsistency at the start. But I think we
should drive towards checking `isSupported` rather than `isEnabled`. I don't
think we will have many cases for `isEnabled` at the end.
================
Comment at: clang/lib/Basic/TargetInfo.cpp:398
+ auto CLVer = Opts.OpenCLCPlusPlus ? 200 : Opts.OpenCLVersion;
+ if (CLVer >= 300) {
+ auto &OCLOpts = getSupportedOpenCLOpts();
----------------
azabaznov wrote:
> Anastasia wrote:
> > I suggest we move this onto `OpenCLOptions::addSupport`.
> This should stay here to control simultaneous macro definitions
Could we leave this bit out? These are set correctly by the targets already...
and I think targets do need to set those explicitly indeed. I don't see big
value in this functionality right now.
================
Comment at: clang/lib/Headers/opencl-c.h:4635
-#ifdef cl_khr_fp64
+#if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
#pragma OPENCL EXTENSION cl_khr_fp64 : enable
----------------
azabaznov wrote:
> azabaznov wrote:
> > Anastasia wrote:
> > > svenvh wrote:
> > > > Wondering if we need a similar change in
> > > > `clang/lib/Headers/opencl-c-base.h` to guard the double<N> vector types?
> > > Instead of growing the guard condition, I suggest we only check for one
> > > for example the feature macro and then make sure the feature macro is
> > > defined in `opencl-c-base.h` if the extensions that aliases to the
> > > feature is supported. However, it would also be ok to do the opposite
> > > since the extensions and the corresponding features should both be
> > > available.
> > >
> > > FYI, something similar was done in https://reviews.llvm.org/D92004.
> > >
> > > This will also help to propagate the functionality into Tablegen header
> > > because we won't need to extend it to work with multiple extensions but
> > > we might still need to do the rename.
> > Yeah, I overlooked this place... Thanks!
> I don't think that growing of this condition will hurt in some cases... Note,
> that unifying condition check makes sense if some features are
> unconditionally supported for OpenCL C 2.0, such as generic address space for
> example. In other cases (such as fp64, 3d image writes, subgroups) we should
> use OR in guard condition.
>
> Your approach will require extra checks in clang such as, for example, to
> make sure that extension macro is not predefined if the feature macro is
> defined, because there will be redefinition since extension macro is defined
> in header.
I think using one macro for everything is just simpler and cleaner.
> Your approach will require extra checks in clang such as, for example, to
> make sure that extension macro is not predefined if the feature macro is
> defined, because there will be redefinition since extension macro is defined
> in header.
I think we should handle this in the headers instead and we should definitely
define the macros conditionally to avoid redefinitions.
================
Comment at: clang/test/SemaOpenCL/opencl-fp64-feature.cl:1
+// Test with a target not supporting fp64.
+// RUN: %clang_cc1 -cl-std=CL3.0 %s -triple r600-unknown-unknown -target-cpu
r600 -verify -pedantic -fsyntax-only -DNOFP64
----------------
azabaznov wrote:
> Anastasia wrote:
> > I suggest we merge this with `extensions.cl` that has similar
> > functionality. As it mainly tests doubles I don't mind if you prefer to
> > rename it then.
> Sure, I'll try that. At first, I just didn't want to modify test's guard
> checks as it will become messy:
>
> ```
> #if __OPENCL_C_VERSION__ >= 300
> // expected-error@-2{{use of type 'double' requires __opencl_c_fp64 feature
> to be enabled}}
> #else
> // expected-error@-4{{use of type 'double' requires cl_khr_fp64 feature to be
> enabled}}
> #endif
> ```
>
> I see if I can avoid that. Maybe also try to unify the diagnostic for that
> case?
I agree, perhaps regular expressions in the diagnostics could help too i.e.
`expected-error-re`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96524/new/
https://reviews.llvm.org/D96524
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
