[PATCH] D103241: [OpenCL] Add memory_scope_all_devices

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov 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:
> svenvh wrote:
> > 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.
> FYI we have already added extended subgroups extension macros for SPIR in `opencl-c-base.h` without the `__undef<...>` trick.
> 
> ```
> #if defined(__SPIR__)
> #define cl_khr_subgroup_extended_types 1
> #define cl_khr_subgroup_non_uniform_vote 1
> #define cl_khr_subgroup_ballot 1
> #define cl_khr_subgroup_non_uniform_arithmetic 1
> #define cl_khr_subgroup_shuffle 1
> #define cl_khr_subgroup_shuffle_relative 1
> #define cl_khr_subgroup_clustered_reduce 1
> #endif // defined(__SPIR__)
> ```
> 
> But extra conditions can be added any time if we get the agreement on the route forward. 
> 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?

Well, IMO separating extensions/features into two classes of options exactly brings new complexities :) I'm not sure why do we need to have a separate interface for them if there already exists unified one. For example, Intel compute-runtime uses `-cl-ext` flag to forward options : https://github.com/intel/compute-runtime/blob/master/opencl/source/platform/extensions.cpp#L156. 

Can we use this header  mechanism  to define header-only features/extensions while `-cl-ext`interface is preserved?


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

https://reviews.llvm.org/D103241