azabaznov marked 3 inline comments as done.
azabaznov 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");
+}
----------------
Anastasia wrote:
> 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.
Thanks for feedback. I did some refactoring needed for this patch to proceed:
https://reviews.llvm.org/D97058. AFAIU it doesn't conflict with the one you did
(https://reviews.llvm.org/D97052).
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
