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:
> 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...
> 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...

I see the main advantage is having the unified interface for all type of 
extensions/features: either it affects language semantics or it is a 
header-only. Also, I don't think it requires a lot of changes in clang: we only 
need to extend `-cl-ext` to add unknown extensions/features (which are not in 
`OpenCLExtensions.def`) and add a flag that extension/feature is header-only. 
Alternatively, we could add a new mode when compiling for SPIR target when not 
support all the features/extensions to not use the `-D__undef` trick. I think 
we can proceed with this and have further discussions.


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

https://reviews.llvm.org/D103241

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to