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

[Anastasia Stulova via cfe-commits]

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 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. When I absorbed __attribute__((overloadable)) into __const_func to save space, it did not sacrifice readability. However using macro with gentype will cause the header file more difficult to read.
> >
> >
> > But I don't think this kind of so long header is easy to read, it contains full pages of the same function with different types and it is hard to see if anyone is missing or find where these functions end. In spec builtin functions can be represented by
> >
> > > gentype ctz (gentype x)
> >
> > >  gentype max (gentype x, gentype y)
> >
> > >  gentype max (gentype x, sgentype y)
> >
> >
> > If we could use some macro or script to generate the actual builtin functions, it seems more likely spec and clear to read, also will avoid missing or typo.
>
>
> For simple cases, we could define something like this:
>
> #define MATH_FUN(F) \
>  float F(float x); \
>  double F(double x); \
>  half F(half x);
>
> MATH_FUN(sin)
>  MATH_FUN(cos)
>
> It could be concise without sacrificing much clarity. However, for more complicated patterns, like
>
> uchar8 __const_func convert_uchar8_rtn(int8);
>
> It seems hard to balance between readability and brevity. Mostly it will ends up with multiple layers of macros calling each other, and the original function declarations obscured. This also opens Pandora's box of subjectivity of readability.
>
> I think using macro and not using macro both have advantages and disadvantages. That's why I did not take efforts to change one representation to another.
>
> It will take considerable efforts to define all builtin functions as macros, whereas this review already dragged too long.
>
> Anastasia, what's your suggestion? Thanks.


Let's don't exaggerate with macros for this. Using them has negative effect of diagnostics being reported about a macro and not the original function, which won't conform the Spec any longer.

See my earlier comment to line 6582:

  error: too many arguments provided to function-like macro invocation


================
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)) async_work_group_copy(__local char3 *dst, const __global char3 *src, size_t num_elements, event_t event);
+event_t __attribute__((overloadable)) async_work_group_copy(__local uchar3 *dst, const __global uchar3 *src, size_t num_elements, event_t event);
----------------
yaxunl wrote:
> If this representation is not generic enough. Any suggestion for an alternative? Thanks.
I don't think Spec imposes any specific implementation of this macro.

I am thinking we might better leave it out to allow adding in a way suitable for other implementations.


http://reviews.llvm.org/D18369