Hi Pekka,<br>thank you for the reply and for the paper, I have never heard of SAP before,<br>very interesting field.<br><br><br>Hi Peter, <br><br>the suggestion you gave my are what I am looking for, thank you very much.<br>
<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
- Please make sure your patch conforms with the LLVM coding standards<br>
(<a href="http://llvm.org/docs/CodingStandards.html" target="_blank">http://llvm.org/docs/CodingStandards.html</a>). In particular indentation<br>
is inconsistent in some areas.<br></blockquote><div><br>The problem was the usage of tabs instead of spaces, fixed.<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
- Similarly, your patch reformats parts of the Clang source code in a<br>
way that is inconsistent with the coding standards. Please remove<br>
these changes.<br>
<br>
- For such a large change as this, it would be really nice (and<br>
easier to review) if your patch were broken up into a series of<br>
separate logical changes. This is easy to do using git; I saw<br>
you're using hg so this should also be possible.<br></blockquote><div><br>Ok, I'll look for it in Mercurial. <br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
- You added an OpenCLMask field to Qualifiers for storing the OpenCL<br>
address space. This doubles the size of the Qualifiers object.<br>
Is there any reason why you did not use the existing AddrSpace<br>
subfield? (The way I imagine it could work is to define fixed<br>
address space constants for the constant, global, local and private<br>
address spaces). </blockquote><div><br>I added a new mask for the sake of simplicity, but I think I'll follow your advice.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
You also don't need to use a bitmask (at least<br>
at the AST level) since the address spaces are mutually exclusive.<br></blockquote><div><br>This is a problem on which I thought a lot. Since this qualifiers are mutually<br>exclusive I could have used only two bits to save one of the possible four 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 spaces}}<br><br>The compilation error is due to the presence of a different OpenCL qualifier both<br>
in the typdef both in the variable definition. Now, what is the canonical 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 "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 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 already an OpenCL<br>
qualifiers: (__local).<br><br>To overcome this I allowed the possibility to have an inconsistent state in the Qualifiers<br>object, ie 2 bits in the OpenCL qualifiers mask, with the assurance that this inconsistency<br>would be checked in the method Sema::ActOnVariableDeclaration, also called 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 this would be<br>misplaced since all the semantic checks on a variable declaration should be performed in<br>
ActOnVariableDeclarator.<br><br><br>Right now I have no idea how to solve this issue.<br><br>Any suggestion is greatly appreciated.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<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></blockquote><div><br>Sorry for the stupid question, but what is the advantage of using an<br>attribute instead of a function specifier ?<br> </div><br>Thank you very much again.<br>
<br>Bye bye. <br><br>Alberto<br></div><br>