[PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL header file.

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: lib/Headers/opencl-c.h:7452
@@ +7451,3 @@
+
+// OpenCL v1.2 s6.12.2, v2.0 s6.13.2 - Math functions
+
----------------
Could you put OpenCL v1.1 section too?

================
Comment at: lib/Headers/opencl-c.h:7767
@@ +7766,3 @@
+/**
+ * Round to integral value using the round to +ve
+ * infinity rounding mode.
----------------
What does +ve mean?

================
Comment at: lib/Headers/opencl-c.h:7795
@@ +7794,3 @@
+ * Returns x with its sign changed to match the sign of
+ * y.
+ */
----------------
Could y be merged with the previous line?

================
Comment at: lib/Headers/opencl-c.h:7952
@@ +7951,3 @@
+/**
+ * Compute the base- e exponential of x.
+ */
----------------
Can't read this comment.

Perhaps: "Compute base e exponent"?

================
Comment at: lib/Headers/opencl-c.h:8306
@@ +8305,3 @@
+#else
+float __attribute__((overloadable)) fract(float x, __global float *iptr);
+float2 __attribute__((overloadable)) fract(float2 x, __global float2 *iptr);
----------------
Does this mean that all non-generic versions are not available in CL2.0 forcing the dynamic AS conversion for all BIFs with a pointer type?

Wondering if adding those unconditionally would be better thing to do...

================
Comment at: lib/Headers/opencl-c.h:8457
@@ +8456,3 @@
+/**
+ * Compute the value of the square root of x^2+ y^2
+ * without undue overflow or underflow.
----------------
Could you add space please: x^2 + y^2

================
Comment at: lib/Headers/opencl-c.h:9110
@@ +9109,3 @@
+ */
+float __const_func remainder(float x, float y);
+float2 __const_func remainder(float2 x, float2 y);
----------------
Overloadable here and in other places too?

================
Comment at: lib/Headers/opencl-c.h:12772
@@ +12771,3 @@
+#ifdef cl_khr_fp16
+half __attribute__((overloadable)) vload(size_t offset, const half *p);
+half2 __attribute__((overloadable)) vload2(size_t offset, const half *p);
----------------
Should it be vload_half instead?

================
Comment at: lib/Headers/opencl-c.h:14484
@@ +14483,3 @@
+long __attribute__((overloadable)) atom_sub(volatile __global long *p, long val);
+unsigned long __attribute__((overloadable)) atom_sub(volatile __global unsigned long *p, unsigned long val);
+long __attribute__((overloadable)) atom_sub(volatile __local long *p, long val);
----------------
btw, perhaps it's a good idea to have a macro for __attribute__((overloadable)). This should be relatively simple to replace everywhere.

================
Comment at: lib/Headers/opencl-c.h:14766
@@ +14765,3 @@
+
+ATOMIC_INIT_PROTOTYPE(int)
+ATOMIC_INIT_PROTOTYPE(uint)
----------------
Let's not have macros for function declarations. As commented earlier they can break proper diagnostics reporting (ie. diagnostics will display macro instead of actual function).

================
Comment at: lib/Headers/opencl-c.h:15674
@@ +15673,3 @@
+/**
+ * Use the coordinate (coord.z) to index into the
+ * 2D image array object and (coord.x, coord.y) to do an
----------------
I feel that some bits of this description are being repeated multiple times. Could we have a common description at the beginning instead that would cover all the different cases.

================
Comment at: lib/Headers/opencl-c.h:16220
@@ +16219,3 @@
+/**
+ * Write color value to location specified by coordinate
+ * (coord.x) in the 1D image object specified by index
----------------
The same here. Could we unify the description for all write_image functions somehow?

================
Comment at: lib/Headers/opencl-c.h:16960
@@ +16959,3 @@
+
+#define WG_BROADCAST_1D_DECL(type) \
+type __attribute__((overloadable)) work_group_broadcast(type a, size_t local_id);
----------------
Let's remove macros from here too.

================
Comment at: lib/Headers/opencl-c.h:17051
@@ +17050,3 @@
+#define CLK_SUCCESS                                 0
+#define CLK_ENQUEUE_FAILURE                         -101
+#define CLK_INVALID_QUEUE                           -102
----------------
Are those arbitrary taken values I am guessing?

================
Comment at: lib/Headers/opencl-c.h:17099
@@ +17098,3 @@
+
+// ToDo: Add enqueue_kernel as a Clang builtin because it requires custom check of type of variadic 
+// arguments as well as block arguments.
----------------
Could we remove the enqueue_kernel as it's being added as Clang builtin? It's prototype is incorrect here anyways.

Btw, I have started the review for it: http://reviews.llvm.org/D20249

================
Comment at: lib/Headers/opencl-c.h:17252
@@ +17251,3 @@
+/**
+ * Use the coordinate (x) to do an element lookup in
+ * the mip-level specified by the Level-of-Detail (lod)
----------------
I think this should be better grouped with other image functions above.


http://reviews.llvm.org/D18369