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