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


================
Comment at: clang/lib/Headers/opencl-c-base.h:44
+#if (__OPENCL_C_VERSION__ == 300)
+#define __opencl_c_atomic_scope_all_devices 1
+#endif
----------------
I think we should at least add a `defined(__SPIR__)` check though? Otherwise it 
won't be correct for other targets using the same header.


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