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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
