[cfe-dev] OpenCL support

[Tanya Lattner]

On Nov 29, 2010, at 6:13 PM, Peter Collingbourne wrote:

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

I agree with this point as well.

Also, please resend your patch without the unrelated formatting changes and break it up into incremental patches. That will make it much easier to review and get in.

-Tanya