[PATCH] D103241: [OpenCL] Add memory_scope_all_devices

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh 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)
----------------
Anastasia wrote:
> 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?
> This feature is header only.

Good catch!  I have updated the patch to define the feature macro in the header instead.  Currently that definition is not optional, since we don't have finalized the solution for handling this yet (though the __undef proposal seems to be compatible with this change).

> I personally would like to introduce new flag for OpenCL options in OpenCLExtensions.def which will indicate that feature/extension is header-only

If we still need to add header-only features to OpenCLExtensions.def, then they aren't really header-only anymore I'd argue (as @Anastasia pointed out above).  So I'm not sure we need it either, or perhaps I missed something.


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

https://reviews.llvm.org/D103241