[PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

Regarding half types since there is inconsistency in both headers (commented in CL1.2), should we just enable the extension cl_khr_fp16 in the header and then have the overloads with half there with all the other types? They shouldn't be visible to custom code unless the same extension is enabled in the compiled cl file because half type itself won't be allowed without enabling it.

What about OpenCL 1.1 header? Ideally it would be nice to have them in too!

Is there any chance we could factor out the common bits into separate files to avoid large code repetition? I would imagine it should be quite doable as libs of each standard contain incremental changes.

Do you plan integrating it into the Clang driver too by automatic inclusion since it's not done with normal #include?


================
Comment at: lib/Headers/opencl-12.h:585
@@ +584,3 @@
+double16 const_func __attribute__((overloadable)) cbrt(double16);
+//half const_func __attribute__((overloadable)) cbrt(half);
+//half2 const_func __attribute__((overloadable)) cbrt(half2);
----------------
Why commented code here?

================
Comment at: lib/Headers/opencl-20.h:4150
@@ +4149,3 @@
+ */
+#define as_char(x) __builtin_astype((x), char)
+#define as_char2(x) __builtin_astype((x), char2)
----------------
I think we should have a normal declaration of these BIFs, because otherwise this won't appear as a symbol anywhere and would prevent for example error reporting with the original OpenCL function name.

An implementation of the builtin function can just call the Clang builtin __builtin_astype in its definition. This is also more general approach in case some implementations will decide to do something else here. 

================
Comment at: lib/Headers/opencl-20.h:9866
@@ +9865,3 @@
+
+// TODO: fast_normalize(half)?
+
----------------
There is a TODO here!

================
Comment at: lib/Headers/opencl-20.h:11213
@@ +11212,3 @@
+#define PIPE_RESERVE_ID_VALID_BIT (1U << 30)
+#define CLK_NULL_RESERVE_ID (__builtin_astype(((void*)(~PIPE_RESERVE_ID_VALID_BIT)), reserve_id_t))
+bool __attribute__((overloadable)) is_valid_reserve_id(reserve_id_t reserve_id);
----------------
This doesn't come directly from Spec.

================
Comment at: lib/Headers/opencl-20.h:11222
@@ +11221,3 @@
+#define MAX_WORK_DIM        3
+typedef struct {
+    unsigned int workDimension;
----------------
This isn't defined by Spec but it seems sensible to define it this way.

However, there is a conflict here as ndrange_t is already added as a Clang builtin type:
https://llvm.org/svn/llvm-project/cfe/trunk@247676
 and it is compiled to opaque type in IR. However, considering that we can have local variables and returns of this type, we might remove it from Clang type then and add it here in a header.

Any thoughts?

================
Comment at: lib/Headers/opencl-20.h:11251
@@ +11250,3 @@
+int __attribute__((overloadable))
+enqueue_kernel(queue_t queue, kernel_enqueue_flags_t flags,
+               const ndrange_t ndrange, void (^block)(local void *, ...),
----------------
I think I would prefer to add an enqueue kernel as a Clang builtin because it requires custom check of type of variadic arguments as well as block arguments.

================
Comment at: lib/Headers/opencl-20.h:11263
@@ +11262,3 @@
+uint __attribute__((overloadable)) get_kernel_work_group_size(void (^block)(void));
+uint __attribute__((overloadable)) get_kernel_work_group_size(void (^block)(local void *, ...));
+uint __attribute__((overloadable)) get_kernel_preferred_work_group_size_multiple(void (^block)(void));
----------------
Also here we need a special check of parameters to block, and therefore it should be added as a Clang builtin.

================
Comment at: lib/Headers/opencl-20.h:11572
@@ +11571,3 @@
+
+#define ATOMIC_VAR_INIT(x) (x)
+
----------------
I think this should be target specific?

================
Comment at: lib/Headers/opencl-20.h:11605
@@ +11604,3 @@
+
+#define atomic_init_prototype(TYPE) \
+atomic_init_prototype_addrspace(TYPE, generic)
----------------
Could you change atomic_init_prototype to upper case letters to match the style?

The same below.

Also it seems like some BIFs are declared with macros and some not. Any system there?

================
Comment at: lib/Headers/opencl-20.h:11886
@@ +11885,3 @@
+
+__global  void* __attribute__((overloadable)) to_global(void*);
+__local   void* __attribute__((overloadable)) to_local(void*);
----------------
This isn't correct prototype according to Spec though. It should take any pointer type and not a void*.

This approach will introduce extra type conversions and might lead to loss of type information.

================
Comment at: lib/Headers/opencl-20.h:13851
@@ +13850,3 @@
+/**
+ * Use the coordinate (x, y) to do an element lookup in
+ * in the mip-level specified by lod in the
----------------
Also there seems to be inconsistency in documentation.


http://reviews.llvm.org/D18369