Anastasia added a comment.
Since it has been in review for quite a while, should we try to commit it ASAP?
I think Richard can give us his feedback later as well.
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
Anastasia accepted this revision.
Anastasia added a comment.
LGTM! 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
yaxunl added inline comments.
Comment at: lib/Headers/opencl-c.h:143
@@ +142,3 @@
+#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#define NULL ((void*)0)
+#endif
Anastasia wrote:
> indentation seems wrong!
fixed.
Anastasia added a comment.
Sam, there are a few small comments, otherwise it seems to be in a good shape.
@rsmith, do you think you could take a look here? The OpenCL side is fine, but
I was just wondering if you see any issue with us adding a header of ~17K
lines. It is all part of OpenCL
pxli168 accepted this revision.
pxli168 added a comment.
LGTM!
I think we may add optimization on this base later.
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Anastasia added a comment.
Looking good already! I just have one general concern about committing 16K
lines of code with no tests. But perhaps testing all the declarations isn't an
option actually.
Could we at least have a test that includes the header and makes sure it's
compiled
yaxunl added inline comments.
Comment at: lib/Headers/opencl-c.h:17051
@@ +17050,3 @@
+#define CLK_SUCCESS 0
+#define CLK_ENQUEUE_FAILURE -101
+#define CLK_INVALID_QUEUE -102
jprice
jprice added inline comments.
Comment at: lib/Headers/opencl-c.h:17051
@@ +17050,3 @@
+#define CLK_SUCCESS 0
+#define CLK_ENQUEUE_FAILURE -101
+#define CLK_INVALID_QUEUE -102
yaxunl
yaxunl added inline comments.
Comment at: lib/Headers/opencl-c.h:17051
@@ +17050,3 @@
+#define CLK_SUCCESS 0
+#define CLK_ENQUEUE_FAILURE -101
+#define CLK_INVALID_QUEUE -102
pxli168 added a comment.
Comment at: lib/Headers/opencl-c.h:9110
@@ +9109,3 @@
+ */
+float __const_func remainder(float x, float y);
+float2 __const_func remainder(float2 x, float2 y);
Anastasia wrote:
> Overloadable here and in other places too?
It seems
Anastasia added inline comments.
Comment at: lib/Headers/opencl-c.h:14057
@@ +14056,3 @@
+event_t __attribute__((overloadable)) async_work_group_copy(__local float2
*dst, const __global float2 *src, size_t num_elements, event_t event);
+event_t __attribute__((overloadable))
Anastasia added inline comments.
Comment at: lib/Headers/opencl-c.h:7452
@@ +7451,3 @@
+
+// OpenCL v1.2 s6.12.2, v2.0 s6.13.2 - Math functions
+
Could you put OpenCL v1.1 section too?
Comment at: lib/Headers/opencl-c.h:7767
@@ +7766,3 @@
+/**
+
yaxunl marked 3 inline comments as done.
Comment at: lib/Headers/opencl-c.h:14057
@@ +14056,3 @@
+event_t __attribute__((overloadable)) async_work_group_copy(__local float2
*dst, const __global float2 *src, size_t num_elements, event_t event);
+event_t
Anastasia added a comment.
In http://reviews.llvm.org/D18369#424617, @yaxunl wrote:
> In http://reviews.llvm.org/D18369#424347, @pxli168 wrote:
>
> > In http://reviews.llvm.org/D18369#422367, @yaxunl wrote:
> >
> > > In http://reviews.llvm.org/D18369#421963, @pxli168 wrote:
> > >
> > > > If we
yaxunl added a comment.
In http://reviews.llvm.org/D18369#424347, @pxli168 wrote:
> In http://reviews.llvm.org/D18369#422367, @yaxunl wrote:
>
> > In http://reviews.llvm.org/D18369#421963, @pxli168 wrote:
> >
> > > If we want to save some space, could we use some macro to expand the
> > >
pxli168 added a comment.
In http://reviews.llvm.org/D18369#422367, @yaxunl wrote:
> In http://reviews.llvm.org/D18369#421963, @pxli168 wrote:
>
> > If we want to save some space, could we use some macro to expand the
> > gentype or some script to expand the gentype into each types when the
yaxunl added inline comments.
Comment at: lib/Headers/opencl-c.h:14057
@@ +14056,3 @@
+event_t __attribute__((overloadable)) async_work_group_copy(__local float2
*dst, const __global float2 *src, size_t num_elements, event_t event);
+event_t __attribute__((overloadable))
yaxunl added a comment.
In http://reviews.llvm.org/D18369#421963, @pxli168 wrote:
> If we want to save some space, could we use some macro to expand the gentype
> or some script to expand the gentype into each types when the clang is build?
We need to balance between space and readability.
pxli168 added a comment.
If we want to save some space, could we use some macro to expand the gentype or
some script to expand the gentype into each types when the clang is build?
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
yaxunl added a comment.
typo. saved 300KB space.
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yaxunl added inline comments.
Comment at: lib/Headers/opencl-c.h:4872
@@ +4871,3 @@
+
+#ifdef cl_khr_fp64
+char __const_func __attribute__((overloadable)) convert_char(double);
Anastasia wrote:
> Interesting, macro has the same name as an extension?
The spec
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__ >=
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
bader added a comment.
In http://reviews.llvm.org/D18369#409011, @yaxunl wrote:
> In http://reviews.llvm.org/D18369#408773, @bader wrote:
>
> > BTW, there is a comment on GitHub that opencl.h might be a bad name for
> > that header, since Khronos group provides the header with the same name,
>
yaxunl added a comment.
In http://reviews.llvm.org/D18369#408773, @bader wrote:
> BTW, there is a comment on GitHub that opencl.h might be a bad name for that
> header, since Khronos group provides the header with the same name, but it
> defines host API. So if some developer is using clang to
bader added inline comments.
Comment at: lib/Headers/opencl.h:2
@@ +1,3 @@
+//
+// SPIR Tools
+//
This comment should be updated.
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks!
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
bader added inline comments.
Comment at: lib/Headers/opencl.h:14476-14479
@@ +14475,6 @@
+
+#ifdef _HAS_READ_WRITE_IMAGE
+float4 __attribute__((overloadable)) read_imagef(read_write image2d_t image,
sampler_t sampler, int2 coord);
+float4 __attribute__((overloadable))
bader added inline comments.
Comment at: lib/Headers/opencl.h:14473-14474
@@ +14472,4 @@
+
+float4 __const_func __attribute__((overloadable)) read_imagef(read_only
image2d_t image, sampler_t sampler, int2 coord);
+float4 __const_func __attribute__((overloadable))
bader added a comment.
Sam, could you also add declarations of samplerless read_image and write_image
built-in functions with read_write qualified images, please?
http://reviews.llvm.org/D18369
___
cfe-commits mailing list
bader added a comment.
To clarify my last comment: I don't think we should define Image Query built-in
functions only for 'read_only' access qualifier in OpenCL 1.x.
It's just bug in OpenCL 1.x specifications. I think the intention was to
provide these built-in functions for both 'read_only'
bader added inline comments.
Comment at: lib/Headers/opencl.h:15372-15373
@@ +15371,4 @@
+ */
+int __const_func __attribute__((overloadable)) get_image_width(image1d_t
image);
+int __const_func __attribute__((overloadable))
get_image_width(image1d_buffer_t image);
+int
32 matches
Mail list logo
