Thank you very much for your help Peter and Tanya.<br><br>I will resend all the patches as soon as I finished applying Peter's suggestions.<br><br>Alberto<br><br><div class="gmail_quote">2010/11/30 Tanya Lattner <span dir="ltr"><<a href="mailto:lattner@apple.com">lattner@apple.com</a>></span><br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div><div></div><div class="h5"><br>
On Nov 29, 2010, at 6:13 PM, Peter Collingbourne wrote:<br>
<br>
> Hi Alberto,<br>
><br>
> On Sun, Nov 28, 2010 at 01:19:44PM +0100, Alberto Magni wrote:<br>
>> You also don't need to use a bitmask (at least<br>
>>> at the AST level) since the address spaces are mutually exclusive.<br>
>>><br>
>><br>
>> This is a problem on which I thought a lot. Since this qualifiers are<br>
>> mutually<br>
>> exclusive I could have used only two bits to save one of the possible four<br>
>> cases.<br>
>> But let's take into consideration this test case:<br>
>><br>
>> typedef __global int global_typedef;<br>
>> __local global_typedef* t4; //expected-error {{multiple named address<br>
>> spaces}}<br>
>><br>
>> The compilation error is due to the presence of a different OpenCL qualifier<br>
>> both<br>
>> in the typdef both in the variable definition. Now, what is the canonical<br>
>> type of "t4" ?<br>
>> "__global __local int".<br>
>> In the method Sema::HandleDeclarator there is a call to:<br>
>><br>
>> TypeSourceInfo *TInfo = GetTypeForDeclarator(D, S);<br>
>><br>
>> GetTypeForDeclarator will build the canonical type of the declarator passed<br>
>> as argument (D).<br>
>> The actual stack trace is:<br>
>><br>
>> HandleDeclarator<br>
>> GetTypeForDeclarator<br>
>> BuildPointerType<br>
>> getPointerType<br>
>> getCanonicalType<br>
>><br>
>> The call to getCanonicalType will result in the building of a new<br>
>> "Qualifiers" object,<br>
>> starting from "__local", the local OpenCL qualifier the "__global" qualifier<br>
>> will also be added.<br>
>> If I add an assert in the Qualifier class to the method that adds the new<br>
>> qualifier (__global):<br>
>><br>
>> assert (there are no previous OpenCL qualifiers here)<br>
>><br>
>> it will for sure signal an error, since the Qualifiers object contains<br>
>> already an OpenCL<br>
>> qualifiers: (__local).<br>
>><br>
>> To overcome this I allowed the possibility to have an inconsistent state in<br>
>> the Qualifiers<br>
>> object, ie 2 bits in the OpenCL qualifiers mask, with the assurance that<br>
>> this inconsistency<br>
>> would be checked in the method Sema::ActOnVariableDeclaration, also called<br>
>> by<br>
>> HandleDeclarator.<br>
>><br>
>> One solution would be to add a check for duplicated OpenCL qualifiers in<br>
>> HandleDeclarator before the call to GetTypeForDeclarator, but I think that<br>
>> this would be<br>
>> misplaced since all the semantic checks on a variable declaration should be<br>
>> performed in<br>
>> ActOnVariableDeclarator.<br>
><br>
> Actually, this is a semantic check on declarators, not variable<br>
> declarations.  Remember that the multiple address space diagnostic<br>
> may arise anywhere a declarator may occur (which includes variable<br>
> declarations but may also include things like cast and sizeof<br>
> expressions).<br>
><br>
>> Right now I have no idea how to solve this issue.<br>
>><br>
>> Any suggestion is greatly appreciated.<br>
><br>
> The best place to perform the check is when building the declarator's<br>
> type in GetTypeForDeclarator.  At that point you have the unqualified<br>
> type and can check its address space.  In fact this is how the existing<br>
> support for address space qualifiers works -- have a look at the<br>
> HandleAddressSpaceTypeAttribute function in SemaType.cpp.  Specifically<br>
> at the top of that function we check that the unqualified type does<br>
> not have an address space.<br>
><br>
> This brings me to my second point, which is that the address<br>
> space qualifiers should be handled internally using AttributeLists.<br>
> This makes the change relatively lightweight, as we can build on the<br>
> existing support for address space qualifiers.  I would recommend<br>
> parsing the OpenCL address space qualifiers into a synthesised<br>
> AttributeList, which can be handled like an address space attribute<br>
> in Sema.  r112939 shows how this was done for the __pascal calling<br>
> convention (which can be written __pascal or __attribute__((pascal))).<br>
><br>
>>> - You added a __kernel field to FunctionDecl.  A better way<br>
>>> of handling this IMO would be to add a new attribute (Attr subclass).<br>
>>> See the include/clang/Basic/Attr.td file.<br>
>>><br>
>><br>
>> Sorry for the stupid question, but what is the advantage of using an<br>
>> attribute instead of a function specifier ?<br>
><br>
> We should only add a new field if we expect it to be used often.<br>
> Fields are quick to access but they increase the size of every<br>
> FunctionDecl object (we're using only 12 bits at the moment but may<br>
> wish to use more in future).  The __kernel qualifier is a relatively<br>
> uncommon qualifier and does not need to be accessed often so it should<br>
> be added as an attribute.<br>
<br>
</div></div>I agree with this point as well.<br>
<br>
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.<br>
<font color="#888888"><br>
-Tanya<br>
<br>
</font></blockquote></div><br>