[PATCH] D96524: [OpenCL] Add support of OpenCL C 3.0 __opencl_c_fp64

[Anton Zabaznov via Phabricator via cfe-commits]

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