[PATCH] D89869: [OpenCL] Define OpenCL feature macros for all versions

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> I guess targets like SPIR will be supporting all features by default?

It sounds confusing for me: can you please elaborate about why does SPIR-V target should support all features/extension by default? If we are compiling OpenCL C 3.0 with optional functionality we most likely want to get an error in FE level if some functionality is not supported by the target, but not in BE after SPIR-V translation.



================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:110
+OPENCLFEAT_INTERNAL(__opencl_c_generic_address_space, 200, ~0U)
+OPENCLFEAT_INTERNAL(__opencl_c_work_group_collective_functions, 200, ~0U)
+OPENCLFEAT_INTERNAL(__opencl_c_atomic_order_acq_rel, 200, ~0U)
----------------
Anastasia wrote:
> Does this need to be in the frontend?
I can remove features which affect only header from this file. But in this case we need to extend '-cl-ext' to register unknown features/extensions (http://lists.llvm.org/pipermail/cfe-dev/2020-October/066932.html). I think this option functionality extending should be done in a separate commit, so we can keep this kind of features here at least for now. What do you think?


================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:121
+OPENCLFEAT_INTERNAL(__opencl_c_fp64, 120, ~0U)
+OPENCLFEAT_INTERNAL(__opencl_c_int64, 100, ~0U)
+OPENCLFEAT_INTERNAL(__opencl_c_images, 100, ~0U)
----------------
Anastasia wrote:
> if we are not going to change clang to make int64 conditional I would suggest we don't add this here for now.
Yes, that sounds reasonable to be.


================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:105
     }
     OptMap[Ext].Supported = V;
   }
----------------
Anastasia wrote:
> I guess we need to make sure that targets can't conditionally support features in OpenCL 2.0 or earlier standards.
This can be done by adding a new method //TargetInfo::setSupportedOpenCL30Features()// which can be called in //::adjust//, does this make sense? I believe now current  options setting (//TargetInfo::setSupportedOpenCLOpts()//) knows nothing about OpenCL version which.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:950
+  if (getLangOpts().OpenCL)
+    getTarget().getSupportedOpenCLOpts().adjustFeatures(getLangOpts());
+
----------------
Anastasia wrote:
> Would it be possible to move this into `getTarget().adjust(getLangOpts())` just below. There is a FIXME that explains that we should be doing such adjustment differently but we haven't solved it with time so let's keep the flow as is for now. 
Yeah, this can be moved there. Btw a lot of OpenCL C 3.0 options setting will take place in //::adjust//...


================
Comment at: clang/test/Preprocessor/opencl-feature-extension-simult.cl:15
+
+// RUN: %clang_cc1 %s -E -cl-std=CL3.0 -cl-ext=-all__opencl_c_fp64
+// RUN: %clang_cc1 %s -E -cl-std=CL3.0 -cl-ext=+__opencl_c_fp64
----------------
Anastasia wrote:
> Is this a typo?
> 
> `all__opencl_c_fp64`
Ah, yeah, thanks, Will definitely change that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89869/new/

https://reviews.llvm.org/D89869