[OpenCL] Add memory_scope_all_devices

[svenvh]

Summary

Add the memory_scope_all_devices enum value, which is restricted to
OpenCL 3.0 or newer and the __opencl_c_atomic_scope_all_devices
feature. Also guard memory_scope_all_svm_devices accordingly, which
is already available in OpenCL 2.0.

The __opencl_c_atomic_scope_all_devices feature is header-only, so
set its define to 1 in opencl-c-base.h. This is done
unconditionally at the moment, as the mechanism for disabling
header-only options hasn't been decided yet.

This patch only adds a negative test for now. Ideally adding a CL3.0
run line to atomic-ops.cl should suffice as a positive test, but we
cannot do that yet until (at least) generic address spaces and program
scope variables are supported in OpenCL 3.0 mode.

[azabaznov]

clang/include/clang/Basic/OpenCLExtensions.def

113 ↗

(On Diff #348222)

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?

[Anastasia]

clang/include/clang/Basic/OpenCLExtensions.def

113 ↗

(On Diff #348222)

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?

[svenvh]

Update change to be header-only.

[svenvh]

clang/include/clang/Basic/OpenCLExtensions.def

113 ↗

(On Diff #348222)

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.

[Anastasia]

clang/include/clang/Basic/OpenCLExtensions.def

113 ↗

(On Diff #348222)

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.

clang/lib/Headers/opencl-c-base.h

44

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.

[azabaznov]

clang/include/clang/Basic/OpenCLExtensions.def

113 ↗

(On Diff #348222)

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-extinterface is preserved?

[svenvh]

Restrict feature macro definition to SPIR target only.

[Anastasia]

clang/include/clang/Basic/OpenCLExtensions.def

113 ↗

(On Diff #348222)

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

[Anastasia]

LGTM! Thanks.

Considering that this patch doesn't add anything new to the design, I suggest we go ahead. But we should continue the discussion around -cl-ext etc elsewhere and then perhaps some follow up patches will be needed to fix this fully.

clang/test/Headers/opencl-c-header.cl

154–160

btw I think we are not testing that the macros are not defined for other targets than SPIR, but I guess this deserves a separate patch.

[azabaznov]

clang/include/clang/Basic/OpenCLExtensions.def

113 ↗

(On Diff #348222)

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.

clang/lib/Headers/opencl-c-base.h

 #define __opencl_c_atomic_scope_device 1
 #define __opencl_c_atomic_scope_all_devices 1
 #define __opencl_c_device_enqueue 1
 #define __opencl_c_read_write_images 1
 #define __opencl_c_program_scope_global_variables 1
 #define __opencl_c_images 1
 #endif
 
+// Define header-only feature macros for OpenCL C 3.0.
+#if (__OPENCL_C_VERSION__ == 300)
+// For the SPIR target all features are supported.
+#if defined(__SPIR__)
+#define __opencl_c_atomic_scope_all_devices 1
+#endif // defined(__SPIR__)
+#endif // (__OPENCL_C_VERSION__ == 300)
+
 // built-in scalar data types:
 
 /**
  * An unsigned 8-bit integer.
  */
 typedef unsigned char uchar;
 
 /**

 #define CLK_GLOBAL_MEM_FENCE   0x02
 
 #if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 
 typedef enum memory_scope {
   memory_scope_work_item = __OPENCL_MEMORY_SCOPE_WORK_ITEM,
   memory_scope_work_group = __OPENCL_MEMORY_SCOPE_WORK_GROUP,
   memory_scope_device = __OPENCL_MEMORY_SCOPE_DEVICE,
+#if defined(__opencl_c_atomic_scope_all_devices)
   memory_scope_all_svm_devices = __OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES,
+#if (__OPENCL_C_VERSION__ >= CL_VERSION_3_0)
+  memory_scope_all_devices = memory_scope_all_svm_devices,
+#endif // __OPENCL_C_VERSION__ >= CL_VERSION_3_0
+#endif // defined(__opencl_c_atomic_scope_all_devices)
 #if defined(cl_intel_subgroups) || defined(cl_khr_subgroups)
   memory_scope_sub_group = __OPENCL_MEMORY_SCOPE_SUB_GROUP
 #endif
 } memory_scope;
 
 /**
  * Queue a memory fence to ensure correct ordering of memory
  * operations between work-items of a work-group to

clang/test/Headers/opencl-c-header.cl

 #endif
 #ifdef cl_khr_subgroup_clustered_reduce
 #error "Incorrect cl_khr_subgroup_clustered_reduce define"
 #endif
 
 #endif //(defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
 
 // OpenCL C features.
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ == 200)
+#if (__OPENCL_C_VERSION__ == 300)
+
+#if __opencl_c_atomic_scope_all_devices != 1
+#error "Incorrectly defined feature macro __opencl_c_atomic_scope_all_devices"
+#endif
+
+#elif (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ == 200)
 
 #ifndef  __opencl_c_pipes
 #error "Feature macro __opencl_c_pipes should be defined"
 #endif
 #ifndef __opencl_c_generic_address_space
 #error "Feature macro __opencl_c_generic_address_space should be defined"
 #endif
 #ifndef __opencl_c_work_group_collective_functions

clang/test/SemaOpenCL/atomic-ops.cl

 // RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
 // RUN:   -fsyntax-only -triple=spir64 -fdeclare-opencl-builtins -finclude-default-header
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
 // RUN:   -triple=amdgcn-amd-amdhsa -fdeclare-opencl-builtins -finclude-default-header
+// TODO: add -cl-std=CL3.0 line when generic and psv are supported.
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 struct S { char c[3]; };
void memory_checks(atomic_int *Ap, int *p, int val) {
   (void)__opencl_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, memory_order_relaxed, memory_scope_work_group);
 }
 
 void synchscope_checks(atomic_int *Ap, int scope) {
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_work_item); // expected-error{{synchronization scope argument to atomic operation is invalid}}
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_work_group);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_device);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_all_svm_devices);
+  (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_all_devices);
+#if __OPENCL_C_VERSION__ < CL_VERSION_3_0
+  // expected-error@-2{{use of undeclared identifier 'memory_scope_all_devices'}}
+  // expected-note@* {{'memory_scope_all_svm_devices' declared here}}
+#endif
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, memory_scope_sub_group);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, scope);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, 10);    //expected-error{{synchronization scope argument to atomic operation is invalid}}
 
   // non-integer memory scope is casted to integer type.
   float fscope;
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, 1.0f);
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, fscope);
   struct S s;
   (void)__opencl_atomic_load(Ap, memory_order_relaxed, s); //expected-error{{passing '__private struct S' to parameter of incompatible type 'int'}}
 }
 
 void nullPointerWarning(atomic_int *Ap, int *p, int val) {
   // The 'expected' pointer shouldn't be NULL.
   (void)__opencl_atomic_compare_exchange_strong(Ap, (void *)0, val, memory_order_relaxed, memory_order_relaxed, memory_scope_work_group); // expected-warning {{null passed to a callee that requires a non-null argument}}
 }