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

[Yaxun Liu via cfe-commits]

yaxunl marked 12 inline comments as done.
yaxunl added a comment.

Changed file name to opencl-c.h and updated comments.

Addressed Anastasia's early comments.

I am thinking we may need several more patches to address remaining issues to keep the changes manageable.


================
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);
----------------
Anastasia wrote:
> Why commented code here?
Fixed.

================
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)
----------------
Anastasia wrote:
> 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. 
as_type is defined as operators in v1.2 s6.2.4.2 instead of as builtin functions in 6.12. 

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

================
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);
----------------
Anastasia wrote:
> This doesn't come directly from Spec.
s6.13.16 requires macro CLK_NULL_RESERVE_ID to be defined.

================
Comment at: lib/Headers/opencl-20.h:11222
@@ +11221,3 @@
+#define MAX_WORK_DIM        3
+typedef struct {
+    unsigned int workDimension;
----------------
Anastasia wrote:
> 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?
We need another patch to address this. For now I suggest keep the clang definition of ndrange_t and remove this from header.

================
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 *, ...),
----------------
Anastasia wrote:
> 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.
need another patch for this work.

================
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));
----------------
Anastasia wrote:
> Also here we need a special check of parameters to block, and therefore it should be added as a Clang builtin.
need another patch

================
Comment at: lib/Headers/opencl-20.h:11572
@@ +11571,3 @@
+
+#define ATOMIC_VAR_INIT(x) (x)
+
----------------
Anastasia wrote:
> I think this should be target specific?
We need a uniform representation for this macro in SPIR. As far as I know this works fine on different platforms. If a target needs special handling of this macro condition macro maybe added.

================
Comment at: lib/Headers/opencl-20.h:11605
@@ +11604,3 @@
+
+#define atomic_init_prototype(TYPE) \
+atomic_init_prototype_addrspace(TYPE, generic)
----------------
Anastasia wrote:
> 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?
Fixed.

The newly added builtin functions of opencl 2.0 are using macros.


================
Comment at: lib/Headers/opencl-20.h:11886
@@ +11885,3 @@
+
+__global  void* __attribute__((overloadable)) to_global(void*);
+__local   void* __attribute__((overloadable)) to_local(void*);
----------------
Anastasia wrote:
> 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.
We need another patch to handle this. Since the argument type allows user defined type, it cannot be declared in header file.

================
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
----------------
Anastasia wrote:
> Also there seems to be inconsistency in documentation.
Fixed.


http://reviews.llvm.org/D18369