[cfe-dev] OpenCL support

[Alberto Magni]

Thank you very much for your help Peter and Tanya.

I will resend all the patches as soon as I finished applying Peter's
suggestions.

Alberto

2010/11/30 Tanya Lattner <lattner at apple.com>

>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20101130/fec26e24/attachment.html>