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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
