Anastasia added a comment.
I would prefer if we try to take a slightly different route i.e. if two
features are identical (e.g. `cl_khr_fp64` and `__opencl_c_fp64`) we make sure
that they are both set identical in Target options or command-line interface
using early check and a diagnostic in the driver. This could be done at the end
of the frontend options setup. Then for the rest of the compilation including
header parsing, we can confidently keep checking only one of them because we
know that they are both set consistently and that they are indeed identical. If
you stick to checking for `cl_khr_fp64` you can avoid most of the modifications
completely.
I have added more details in the code comments. :)
================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:41
+ auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
+ return CLVer >= 300 ? isAvailableOption("__opencl_c_fp64", LO)
+ : isAvailableOption("cl_khr_fp64", LO);
----------------
btw since `cl_khr_fp64` is available in all versions could we not just check
only for it?
================
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:
> > 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.
> There are 2 reasons why it should be here
>
> 1) Simultaneous macro definition
>
> 2) Correct option processing: we need to remove `cl_khr_fp64` macro if
> `-cl-ext=-__opencl_c_fp64` specified
Ok as I said I would prefer to stick to an explicit interface though, i.e.
every feature should be set explicitly
`-cl-ext=-__opencl_c_fp64,-cl_khr_fp64`
as the interface gets too messy otherwise...
I believe we won't have that many of those case - is it just fp64, fp16 and
images?
If you are worried about the inconsistent uses (which is a valid concern!) how
about we just add a check with a diagnostic in clang driver about the
consistency of the setup between the language, command-line options and the
target. We already discussed such check in other use cases - to help avoid
using incorrect language versions that the target has no support.
I think this route will be easier than keeping consistency automatically and
silently overriding the options i.e. we already had bad experiences with bugs
due to such logic. A diagnostic would be very helpful both to application
developers and tooling developers and help to avoid extra complexity in clang
too.
================
Comment at: clang/lib/Headers/opencl-c-base.h:530
-#ifdef cl_khr_fp64
+#if defined(cl_khr_fp64) || defined(__opencl_c_fp64)
#define as_double(x) __builtin_astype((x), double)
----------------
Let's only use one macro as they signal the same. I don't mind which one you
choose perhaps `__opencl_c_fp64` could be used since we can freely define it in
the earliest versions or we can continue using `cl_khr_fp64` then no changes
are needed other then making sure it is defined when `__opencl_c_fp64` is
defined which can be done at the top of this header btw.
================
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
----------------
Anastasia wrote:
> 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.
Do you still plan to address this? FYI this comment is the same as in
`opencl-c-base.h`.
================
Comment at: clang/lib/Sema/Sema.cpp:305
+ llvm::StringRef FP64Feature(
+ getLangOpts().OpenCLVersion >= 300 ? "__opencl_c_fp64" :
"cl_khr_fp64");
if (getLangOpts().OpenCLCPlusPlus || getLangOpts().OpenCLVersion >= 200) {
----------------
But `cl_khr_fp64` is also available if `__opencl_c_fp64` is available. If
anything it should be even more available for backward compatibility. I don't
see the point in checking the same thing twice or checking it differently.
If you are worried you could add some comment/documentation to clarify that
they both have to be set or unset simultaneously and a check in the Clang
driver initialization verifying the consistency in the settings.
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
