[cfe-dev] OpenCL patch

[Peter Collingbourne]

On Fri, Dec 17, 2010 at 06:27:43PM +0100, Alberto Magni wrote:
> Hi everybody,
> 
> I saw the Cuda patch submitted by Peter Collingbourne
> (already included in the svn) and I would like to propose
> an OpenCL patch based on his structure.

There are a number of fundamental differences between CUDA and OpenCL.
The most important one in the context of these patches is that in
CUDA address space attributes only apply to declarations, while in
OpenCL they also apply to declarators (i.e. "types").  So it's not
appropriate to add the address space attributes in SemaDeclAttr.cpp
like we do for CUDA (__kernel can stay there).

Anton Lokhmotov (ARM) has already submitted a patch for address space
attributes which seems closer to being correct, so you may want to
restrict this patch to the __kernel attribute.

> The attached patches add the following features:
> 
> 1) Add ocl_{kernel|constant|global|local|private} attributes
> 2) Checking the type of kernel function arguments: OpenCL 1.1 standard
>     chapter 6.8.k
> 3) Checking kernel function return value, OpenCL 1.1 chapter 6.8.j
> 
> The 004-opecl-test.patch contains relevant test cases.
> In order to check the current implementation the opencl.h file has been
> included in the test case.
> It contains the definition of OpenCL keywords based on ocl_* attributes.
> 
> I did so because in don't know where to place these definitions
> in InitPreprocessor.cpp. I saw that language dependent macros are defined in
> InitializePredefinedMacros, is this the right function to place these
> define commands ?

Another difference between CUDA and OpenCL is that in OpenCL these
attributes are (or should be treated as) keywords while in CUDA
they are implemented as macros.  So we shouldn't need to #define
these keywords in opencl.h.  The implementation-independent part of
opencl.h should, IMO, live in lib/Headers.

> Another problem concerns the OpenCL builtin types (eg intptr_t, uintptr_t, ...).
> I mapped these types, using typedefs, __*_TYPE__,
> eg  typedef __PTRDIFF_TYPE__ ptrdiff_t;
> 
> I think the right way to handle all this preprocessor initializations
> is to include an implicit
> header file with all the required stuff.

We may want to pick and choose certain declarations from stddef.h and
stdint.h for this, or just #include both of them (I'm not sure if the
namespace pollution is important).

> At the moment the provided test cases fails due to the presence of the
> aka construct in
> the diagnostic messages, it unrolls the typedef showing the canonical
> type which depends
> on the current architecture.

That's because types like Ctx.getSizeType() are in fact the native
types (not typedefs) for the current target.  When you perform tests
like this:

> +              || Ctx.hasSameType(Ctx.getSizeType(), ParamType)
> +              || Ctx.hasSameType(Ctx.getPointerDiffType(), ParamType)
> +              || Ctx.hasSameType(Ctx.getIntPointerType(), ParamType)
> +              || Ctx.hasSameType(Ctx.getUnsignedIntPointerType(), ParamType))

you are in fact testing that the canonical types are equivalent,
effectively preventing integer types which happen to match these
particular types from being used.

If you want to disallow these typedefs in kernel parameters
a better approach may be to add a 'kernel poison' attribute on
their typedefs and to recursively check for the poison attribute
in HandleOclKernelAttr.

Thanks,
-- 
Peter