[cfe-dev] OpenCL support

[Alberto Magni]

Hi Pekka,
thank you for the reply and for the paper, I have never heard of SAP before,
very interesting field.


Hi Peter,

the suggestion you gave my are what I am looking for, thank you very much.

- Please make sure your patch conforms with the LLVM coding standards
>  (http://llvm.org/docs/CodingStandards.html).  In particular indentation
>  is inconsistent in some areas.
>

The problem was the usage of tabs instead of spaces, fixed.

>
> - Similarly, your patch reformats parts of the Clang source code in a
>  way that is inconsistent with the coding standards.  Please remove
>  these changes.
>
> - For such a large change as this, it would be really nice (and
>  easier to review) if your patch were broken up into a series of
>  separate logical changes.  This is easy to do using git; I saw
>  you're using hg so this should also be possible.
>

Ok, I'll look for it in Mercurial.

- You added an OpenCLMask field to Qualifiers for storing the OpenCL
>  address space.  This doubles the size of the Qualifiers object.
>  Is there any reason why you did not use the existing AddrSpace
>  subfield?  (The way I imagine it could work is to define fixed
>  address space constants for the constant, global, local and private
>  address spaces).


I added a new mask for the sake of simplicity, but I think I'll follow your
advice.

 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.


Right now I have no idea how to solve this issue.

Any suggestion is greatly appreciated.


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


Thank you very much again.

Bye bye.

Alberto
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20101128/f0c25f06/attachment.html>