OpenCL patch

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

OpenCL patch

Alberto Magni
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.

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 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.

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.

Reviews and suggestions are greatly appreciated.

Many thanks,

Alberto

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

003-kernel-sema.patch (11K) Download Attachment
001-test-environment.patch (1K) Download Attachment
002-kernel-address-space-attributes.patch (2K) Download Attachment
004-opencl-test.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: OpenCL patch

Speziale Ettore
Hi,
I'm a Ph.D. student from Politecnico di Milano. I'm helping Alberto
during its master thesis.

> 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.

I've seen Anton have re-submitted its patches. Alberto patches seem to
be a sub-set of Anton patches, so I think is useless to work on
Alberto's patches if Anton patches will be accepted.

> 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).

OK.

> > 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.

My error! Later, thinking about patches, I've arrived at the same
conclusion, but I hadn't access to Internet for few days and so I can
admit the mistake only now.

> 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.

Good, thank you for the explanation. I think we can start working on
this, using Anton's patches as base -- He don't perform signature
checking. This allow to focus work on a small area, allowing to known
how to write good code for Clang.

Politecnico objective is to build an opencl compiler. I think is better
to hack on clang, because (1) we can learn a lot from a
production-quality compiler, (2) students can see a real compiler
implementation and (3) we will not build another opencl compiler.

Maybe we can coordinate our work, in order to exploit available
resources. Our technical experience is not at clang level, but we want
to improve our skills.

What do you think?
Hi,
[hidden email]

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: OpenCL patch

Peter Collingbourne
Hi Speziale,

On Fri, Dec 24, 2010 at 02:35:30PM +0100, Speziale Ettore wrote:

> Hi,
> I'm a Ph.D. student from Politecnico di Milano. I'm helping Alberto
> during its master thesis.
>
> > 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.
>
> I've seen Anton have re-submitted its patches. Alberto patches seem to
> be a sub-set of Anton patches, so I think is useless to work on
> Alberto's patches if Anton patches will be accepted.

What I meant by this is that the type checking aspect of this patch
can be reworked to be built on top of ARM's __kernel attribute patch,
as you pointed out below.

> > 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.
>
> Good, thank you for the explanation. I think we can start working on
> this, using Anton's patches as base -- He don't perform signature
> checking. This allow to focus work on a small area, allowing to known
> how to write good code for Clang.

> Politecnico objective is to build an opencl compiler. I think is better
> to hack on clang, because (1) we can learn a lot from a
> production-quality compiler, (2) students can see a real compiler
> implementation and (3) we will not build another opencl compiler.

Here at Imperial there is at least 1 other person working on an OpenCL
implementation.  We stand to benefit from OpenCL support in Clang;
at least for my work, production-quality is important in order to
work with real-world kernels.

> Maybe we can coordinate our work, in order to exploit available
> resources. Our technical experience is not at clang level, but we want
> to improve our skills.

It may be best to coordinate with ARM on this, as they are taking
the lead on the implementation work.  I am happy to code review any
proposed patches.

Thanks,
--
Peter
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev