[PATCH] D103241: [OpenCL] Add memory_scope_all_devices

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:113
 OPENCL_OPTIONALCOREFEATURE(__opencl_c_atomic_order_seq_cst, false, 300, OCL_C_30)
+OPENCL_OPTIONALCOREFEATURE(__opencl_c_atomic_scope_all_devices, false, 300, OCL_C_30)
 OPENCL_OPTIONALCOREFEATURE(__opencl_c_subgroups, false, 300, OCL_C_30)
----------------
azabaznov wrote:
> This feature is header only. We had a lot of discussions on that and the main idea was not to declare header only features/extensions in `OpenCLExtensions.def` and use `-D__opencl_c_atomic_scope_all_devices=1` instead, @Anastasia can comment on this.
> 
> I personally would like to introduce new flag for OpenCL options in `OpenCLExtensions.def` which will indicate that feature/extension is header-only, and thus all of such options can be declared in `OpenCLExtensions.def`: if flag is set to true it can be stripped out from the parser. What do you think about this?
Yes, I agree the idea is to align with C/C++ directions for scalability i.e. we should only add what is absolutely necessary to the compiler and implement the rest using libraries - just like regular C and C++. We won't be able to scale if we keep adding everything in the compiler. In fact, we already have a huge scalability issue with our builtin functions. If we look at modern C++ - more than 70% of features are not in the compiler at all.

Would it be possible to do something like suggested here: https://reviews.llvm.org/D91531#change-AKXhB4ko4nAO for `cl_khr_depth_images`?

> I personally would like to introduce new flag for OpenCL options in OpenCLExtensions.def which will indicate that feature/extension is header-only, and thus all of such options can be declared in OpenCLExtensions.def: if flag is set to true it can be stripped out from the parser. What do you think about this?

Hmm, I think the macros should either be declared in the headers or using a flag `-D`. I don't know why would adding them in `OpenCLExtensions.def` be beneficial if we can use conventional approaches? This allows avoiding the complexity and makes things more modular. If we look at the OpenCL vendor extensions for example - we probably don't want them all in one place?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103241