[cfe-dev] OpenCL support

[Peter Collingbourne]

Hi Alberto,

On Sun, Nov 28, 2010 at 01:19:44PM +0100, Alberto Magni wrote:
>  You also don't need to use a bitmask (at least
> >  at the AST level) since the address spaces are mutually exclusive.
> >
> 
> This is a problem on which I thought a lot. Since this qualifiers are
> mutually
> exclusive I could have used only two bits to save one of the possible four
> cases.
> But let's take into consideration this test case:
> 
> typedef __global int global_typedef;
> __local global_typedef* t4; //expected-error {{multiple named address
> spaces}}
> 
> The compilation error is due to the presence of a different OpenCL qualifier
> both
> in the typdef both in the variable definition. Now, what is the canonical
> type of "t4" ?
> "__global __local int".
> In the method Sema::HandleDeclarator there is a call to:
> 
> TypeSourceInfo *TInfo = GetTypeForDeclarator(D, S);
> 
> GetTypeForDeclarator will build the canonical type of the declarator passed
> as argument (D).
> The actual stack trace is:
> 
> HandleDeclarator
> GetTypeForDeclarator
> BuildPointerType
> getPointerType
> getCanonicalType
> 
> The call to getCanonicalType will result in the building of a new
> "Qualifiers" object,
> starting from "__local", the local OpenCL qualifier the "__global" qualifier
> will also be added.
> If I add an assert in the Qualifier class to the method that adds the new
> qualifier (__global):
> 
> assert (there are no previous OpenCL qualifiers here)
> 
> it will for sure signal an error, since the Qualifiers object contains
> already an OpenCL
> qualifiers: (__local).
> 
> To overcome this I allowed the possibility to have an inconsistent state in
> the Qualifiers
> object, ie 2 bits in the OpenCL qualifiers mask, with the assurance that
> this inconsistency
> would be checked in the method Sema::ActOnVariableDeclaration, also called
> by
> HandleDeclarator.
> 
> One solution would be to add a check for duplicated OpenCL qualifiers in
> HandleDeclarator before the call to GetTypeForDeclarator, but I think that
> this would be
> misplaced since all the semantic checks on a variable declaration should be
> performed in
> ActOnVariableDeclarator.

Actually, this is a semantic check on declarators, not variable
declarations.  Remember that the multiple address space diagnostic
may arise anywhere a declarator may occur (which includes variable
declarations but may also include things like cast and sizeof
expressions).

> Right now I have no idea how to solve this issue.
> 
> Any suggestion is greatly appreciated.

The best place to perform the check is when building the declarator's
type in GetTypeForDeclarator.  At that point you have the unqualified
type and can check its address space.  In fact this is how the existing
support for address space qualifiers works -- have a look at the
HandleAddressSpaceTypeAttribute function in SemaType.cpp.  Specifically
at the top of that function we check that the unqualified type does
not have an address space.

This brings me to my second point, which is that the address
space qualifiers should be handled internally using AttributeLists.
This makes the change relatively lightweight, as we can build on the
existing support for address space qualifiers.  I would recommend
parsing the OpenCL address space qualifiers into a synthesised
AttributeList, which can be handled like an address space attribute
in Sema.  r112939 shows how this was done for the __pascal calling
convention (which can be written __pascal or __attribute__((pascal))).

> > - You added a __kernel field to FunctionDecl.  A better way
> >  of handling this IMO would be to add a new attribute (Attr subclass).
> >  See the include/clang/Basic/Attr.td file.
> >
> 
> Sorry for the stupid question, but what is the advantage of using an
> attribute instead of a function specifier ?

We should only add a new field if we expect it to be used often.
Fields are quick to access but they increase the size of every
FunctionDecl object (we're using only 12 bits at the moment but may
wish to use more in future).  The __kernel qualifier is a relatively
uncommon qualifier and does not need to be accessed often so it should
be added as an attribute.

Thanks,
-- 
Peter