[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:
> 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?
I think if we can have a unified interface at no extra cost then sure we should do so. But adding everything into the compiler source code seems like a high cost to me.


> Can we use this header  mechanism  to define header-only features/extensions while -cl-extinterface is preserved?

Perhaps I am missing something but why would using `-cl-ext` be better than `-D`? If you use `-D` you don't need to add anything to clang source code at all so it seems better. Of course, it means that you have to pass an extra flag to the compiler. But it still seems easier than modifying the compiler source code...


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

https://reviews.llvm.org/D103241