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

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: lib/Headers/opencl-c.h:14
@@ +13,3 @@
+#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#define _CL20_AND_ABOVE 1
+#endif
----------------
Is there any reason to define this extra macro?

Could we not just check __OPENCL_C_VERSION__ >= CL_VERSION_2_0

================
Comment at: lib/Headers/opencl-c.h:137
@@ +136,3 @@
+#endif
+typedef double double2 __attribute__((ext_vector_type(2)));
+typedef double double3 __attribute__((ext_vector_type(3)));
----------------
cl_khr_fp64 is not enabled!

================
Comment at: lib/Headers/opencl-c.h:144
@@ +143,3 @@
+#ifdef _CL20_AND_ABOVE
+#define NULL                              0
+#endif
----------------
I think it should be (void*)0.

Btw, I think we should make it available in all OpenCL versions (at least as a part of C99 itself).

================
Comment at: lib/Headers/opencl-c.h:4872
@@ +4871,3 @@
+
+#ifdef cl_khr_fp64
+char __const_func __attribute__((overloadable)) convert_char(double);
----------------
Interesting, macro has the same name as an extension?

================
Comment at: lib/Headers/opencl-c.h:6581
@@ +6580,3 @@
+ */
+#define as_char(x) __builtin_astype((x), char)
+#define as_char2(x) __builtin_astype((x), char2)
----------------
I would like to avoid mapping to Clang builtin in macro because it breaks error reporting (i.e. the error is reported about the macro/Clang builtin and not a proper OpenCL function)

Ex:
  error: too many arguments provided to function-like macro invocation
  as_char(1,1);
                   ^
  note: macro 'as_char' defined here

This also reduces generality in case some implementation need to do something different here.

Such mapping can be simply done in library implementation and should be easily inlined during optimizations.

================
Comment at: lib/Headers/opencl-c.h:13318
@@ +13317,3 @@
+// The current declaration is more tolerant then they should be.
+__global  void* __attribute__((overloadable)) to_global(void*);
+__local   void* __attribute__((overloadable)) to_local(void*);
----------------
Could we remove this functions from here please as they are not properly declared anyways?

I think they should be added as Clang builtins directly.

================
Comment at: lib/Headers/opencl-c.h:14056
@@ +14055,3 @@
+#ifdef _CL20_AND_ABOVE
+#define ATOMIC_VAR_INIT(x) (x)
+
----------------
I am not sure this implementation of macro is generic enough!

================
Comment at: lib/Headers/opencl-c.h:15312
@@ +15311,3 @@
+
+#ifdef _HAS_READ_WRITE_IMAGE
+float4 __attribute__((overloadable)) read_imagef(read_write image1d_t image, int coord);
----------------
Should we just check the CL version instead? The same for other occurrences!


http://reviews.llvm.org/D18369