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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
