Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl marked 4 inline comments as done. Comment at: lib/Headers/opencl.h:15636-15637 @@ +15635,4 @@ +#if defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ >= 200 +#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); yaxunl wrote: > pxli168 wrote: > > Is this macro needed in this header? > > And what happens to spir32 and spir64 difference? > The spec requires to define this macro. > > I agree this definition seems arbitrary since the spec does not define > PIPE_RESERVE_ID_VALID_BIT. > > How about > > // Define an internally used macro for the maximum value of size_t. > #if defined(__SPIR32__) > #define _SIZET_MAX UINT_MAX > #elif defined(__SPIR64__ ) > #define _SIZET_MAX ULONG_MAX > #endif > > and use it for defining CLK_NULL_RESERVE_ID. Actually there is a predefined macro `__SIZE_MAX__` by Clang which is just for this purpose. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl marked 8 inline comments as done. Comment at: lib/Headers/opencl.h:11-13 @@ +10,5 @@ + +#if !defined(__SPIR32__) && !defined(__SPIR64__) +#error "This header file should be used with SPIR target only." +#endif + tstellarAMD wrote: > This should be removed so they can be used with any target. Agree. Comment at: lib/Headers/opencl.h:53-57 @@ +52,7 @@ + */ +#if defined(__SPIR32__) +typedef uint size_t; +#elif defined(__SPIR64__) +typedef ulong size_t; +#endif + tstellarAMD wrote: > This needs to be removed too. clang does not define size_t for OpenCL but defines `__SIZE_TYPE__`, so I can use #define size_t __SIZE_TYPE__ since `__SIZE_TYPE__` is defined based on pointer width of the architecture, we will get a size_t definition consistent with target pointer width. Comment at: lib/Headers/opencl.h:65-69 @@ +64,7 @@ + */ +#if defined(__SPIR32__) +typedef int ptrdiff_t; +#elif defined(__SPIR64__ ) +typedef long ptrdiff_t; +#endif + tstellarAMD wrote: > And this. Clang does not define ptrdiff_t for OpenCL but I can define it as `__PTRDIFF_TYPE__` which is defined by Clang. Comment at: lib/Headers/opencl.h:70-84 @@ +69,17 @@ +#endif + +/** +* A signed integer type with the property that any valid pointer to +* void can be converted to this type, then converted back to pointer +* to void, and the result will compare equal to the original pointer. +*/ +typedef ptrdiff_t intptr_t; + +/** +* An unsigned integer type with the property that any valid pointer to +* void can be converted to this type, then converted back to pointer +* to void, and the result will compare equal to the original pointer. +*/ +typedef size_t uintptr_t; + +// built-in vector data types: tstellarAMD wrote: > Do we actually need these typdefs? I thought clang automatically set these > for OpenCL. Clang does not define intptr_t nor uintptr_t, but it defines `__INTPTR_TYPE__` and `__UINTPTR_TYPE__`. I can use them. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
tstellarAMD added inline comments. Comment at: lib/Headers/opencl.h:11-13 @@ +10,5 @@ + +#if !defined(__SPIR32__) && !defined(__SPIR64__) +#error "This header file should be used with SPIR target only." +#endif + This should be removed so they can be used with any target. Comment at: lib/Headers/opencl.h:53-57 @@ +52,7 @@ + */ +#if defined(__SPIR32__) +typedef uint size_t; +#elif defined(__SPIR64__) +typedef ulong size_t; +#endif + This needs to be removed too. Comment at: lib/Headers/opencl.h:65-69 @@ +64,7 @@ + */ +#if defined(__SPIR32__) +typedef int ptrdiff_t; +#elif defined(__SPIR64__ ) +typedef long ptrdiff_t; +#endif + And this. Comment at: lib/Headers/opencl.h:70-84 @@ +69,17 @@ +#endif + +/** +* A signed integer type with the property that any valid pointer to +* void can be converted to this type, then converted back to pointer +* to void, and the result will compare equal to the original pointer. +*/ +typedef ptrdiff_t intptr_t; + +/** +* An unsigned integer type with the property that any valid pointer to +* void can be converted to this type, then converted back to pointer +* to void, and the result will compare equal to the original pointer. +*/ +typedef size_t uintptr_t; + +// built-in vector data types: Do we actually need these typdefs? I thought clang automatically set these for OpenCL. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl marked 4 inline comments as done.
Comment at: lib/Headers/opencl.h:13721-13726
@@ +13720,8 @@
+
+/**
+ * Queue a memory fence to ensure correct ordering of memory
+ * operations between work-items of a work-group to
+ * image memory.
+ */
+#define CLK_IMAGE_MEM_FENCE 0x04
+
pxli168 wrote:
> Move this to the barrier part with worg_group_barrier maybe better.
Will do.
Comment at: lib/Headers/opencl.h:15636-15637
@@ +15635,4 @@
+#if defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ >= 200
+#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);
pxli168 wrote:
> Is this macro needed in this header?
> And what happens to spir32 and spir64 difference?
The spec requires to define this macro.
I agree this definition seems arbitrary since the spec does not define
PIPE_RESERVE_ID_VALID_BIT.
How about
// Define an internally used macro for the maximum value of size_t.
#if defined(__SPIR32__)
#define _SIZET_MAX UINT_MAX
#elif defined(__SPIR64__ )
#define _SIZET_MAX ULONG_MAX
#endif
and use it for defining CLK_NULL_RESERVE_ID.
Comment at: lib/Headers/opencl.h:15661
@@ +15660,3 @@
+#define CLK_NULL_QUEUE 0
+#define CLK_NULL_EVENT (__builtin_astype(((void*)(UINT32MAX)), clk_event_t))
+
pxli168 wrote:
> What is this UINT32MAX?
> And what about spir64?
How about replacing UINT32MAX with _SIZET_MAX defined above?
Comment at: lib/Headers/opencl.h:15675
@@ +15674,3 @@
+#define MAX_WORK_DIM3
+#if defined(__clang__) && (__clang_major__ < 3 || (__clang_major__ == 3 &&
__clang_minor__ < 9))
+typedef struct {
pxli168 wrote:
> Is this part necessary for up-streaming to the llvm3.9? It will never be hint.
I will remove it.
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
pxli168 added inline comments.
Comment at: lib/Headers/opencl.h:13721-13726
@@ +13720,8 @@
+
+/**
+ * Queue a memory fence to ensure correct ordering of memory
+ * operations between work-items of a work-group to
+ * image memory.
+ */
+#define CLK_IMAGE_MEM_FENCE 0x04
+
Move this to the barrier part with worg_group_barrier maybe better.
Comment at: lib/Headers/opencl.h:15636-15637
@@ +15635,4 @@
+#if defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ >= 200
+#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);
Is this macro needed in this header?
And what happens to spir32 and spir64 difference?
Comment at: lib/Headers/opencl.h:15661
@@ +15660,3 @@
+#define CLK_NULL_QUEUE 0
+#define CLK_NULL_EVENT (__builtin_astype(((void*)(UINT32MAX)), clk_event_t))
+
What is this UINT32MAX?
And what about spir64?
Comment at: lib/Headers/opencl.h:15675
@@ +15674,3 @@
+#define MAX_WORK_DIM3
+#if defined(__clang__) && (__clang_major__ < 3 || (__clang_major__ == 3 &&
__clang_minor__ < 9))
+typedef struct {
Is this part necessary for up-streaming to the llvm3.9? It will never be hint.
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
pxli168 added a comment. I think merge them into one file is a good idea. And the layout by sepc order is OK, is will makes the review easy. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
Anastasia added a comment. In http://reviews.llvm.org/D18369#393323, @yaxunl wrote: > We need to discuss the layout of the header file. > > For the builtin functions, I plan to follow the order of the spec and > extension spec. If there are difference in 1.2 and 2.0, use condition macro. > > For functions with double or half arguments, we have two options: > > 1. keep all functions with double arguments as one section, keep all > functions with half arguments as another section. This is the current layout > of opencl-20.h > 2. keep functions with the same name together so that they follow the spec > order. > > Any suggestions? Thanks. I am fine with either option actually. Thanks, Anastasia http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl added a comment. We need to discuss the layout of the header file. For the builtin functions, I plan to follow the order of the spec and extension spec. If there are difference in 1.2 and 2.0, use condition macro. For functions with double or half arguments, we have two options: 1. keep all functions with double arguments as one section, keep all functions with half arguments as another section. This is the current layout of opencl-20.h 2. keep functions with the same name together so that they follow the spec order. Any suggestions? Thanks. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
Anastasia added a comment. In http://reviews.llvm.org/D18369#389469, @yaxunl wrote: > One of the difference between opencl-12.cl and opencl-20.cl is opencl-12.cl > defines > > #define const_func __attribute__((const)) > #define readonly __attribute__((pure)) > > > and uses them for many functions, e.g. > > float const_func __attribute__((overloadable)) acos(float); > > > I think this is a nice feature for performance. However surprisingly > opencl-20.cl does not do that. > > I suggest to keep these attributes in the merged file. What do you think? > Thanks. I think it's OK to have them them in. But if we keep this, could we use "__" prefix to avoid possible clashes with the custom code identifiers (the same should apply to any other identifiers which we declare in this header that is not specified in Spec). > Do you agree that we should have one single opencl.h instead of headers for > different OpenCL versions? The only concern about having one header would be its parsing speed due to large size. But I believe that the proportion of CL1.2 and CL2.0 declarations isn't really large. So perhaps we won't gain much from separating into multiple. > > What about OpenCL 1.1 header? Ideally it would be nice to have them in > > too! > > We can add it later after we done with 1.2 and 2.0 headers. Since CL1.2 and CL2.0 are derived from CL1.1, it would make sense to fix the declarations (header) for CL1.1 too. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl added a comment. One of the difference between opencl-12.cl and opencl-20.cl is opencl-12.cl defines #define const_func __attribute__((const)) #define readonly __attribute__((pure)) and uses them for many functions, e.g. float const_func __attribute__((overloadable)) acos(float); I think this is a nice feature for performance. However surprisingly opencl-20.cl does not do that. I suggest to keep these attributes in the merged file. What do you think? Thanks. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl added a comment. Anastasia/Alexey/Xiuli, Do you agree that we should have one single opencl.h instead of headers for different OpenCL versions? Since most 1.2 builtins are included in 2.0. I think this is doable. If no objection, I will try to merge them into one header first then addressing other issues. Thanks. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl added a comment. > Rather than having a separate header for each version with another header for > the common parts, couldn't we just have a single header that uses #if guards > for the version specific functionality? e.g. > > size_t __attribute__((overloadable)) get_global_id(uint dim); > > #if __OPENCL_C_VERSION__ >= 200 > size_t __attribute__((overloadable)) get_global_linear_id(void); > #endif > > > This also allows an easy path for having OpenCL 1.1 and 1.0 support without > having to add more copies of the header. I think it is a good idea. It simplifies the file names and organization. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
jprice added a subscriber: jprice. jprice added a comment. In http://reviews.llvm.org/D18369#387560, @yaxunl wrote: > In http://reviews.llvm.org/D18369#385799, @Anastasia wrote: > > > 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. > > > I saw some inconsistencies in the common part of the 1.2 and 2.0 headers. I > will try to consolidate them first then try to split. Rather than having a separate header for each version with another header for the common parts, couldn't we just have a single header that uses #if guards for the version specific functionality? e.g. size_t __attribute__((overloadable)) get_global_id(uint dim); #if __OPENCL_C_VERSION__ >= 200 size_t __attribute__((overloadable)) get_global_linear_id(void); #endif This also allows an easy path for having OpenCL 1.1 and 1.0 support without having to add more copies of the header. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
yaxunl added a comment. In http://reviews.llvm.org/D18369#385799, @Anastasia wrote: > 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. The 2.0 header uses #ifdef cl_khr_fp16 for half builtins, we could do the same for 1.2 headers. > What about OpenCL 1.1 header? Ideally it would be nice to have them in too! We can add it later after we done with 1.2 and 2.0 headers. > 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. I saw some inconsistencies in the common part of the 1.2 and 2.0 headers. I will try to consolidate them first then try to split. > Do you plan integrating it into the Clang driver too by automatic inclusion > since it's not done with normal #include? Yes. http://reviews.llvm.org/D18369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18369: [OpenCL] Upstreaming khronos OpenCL 1.2/2.0 header files.
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_DIM3
+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.
