[cfe-dev] OpenCL support

[Peter Collingbourne]

Hi Alberto,

This is great.  I'm working on a project that uses Clang's OpenCL
frontend, and this will be very useful to me.  I read your patch and
have a few comments below:

- Your patch does not apply cleanly against the current Clang sources
  in subversion.  This is a must if you would like your changes to
  be applied upstream.

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

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

- 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).  You also don't need to use a bitmask (at least
  at the AST level) since the address spaces are mutually exclusive.

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

I also have a few comments on code generation below:

> adding metadata for __kernel functions

The metadata for kernel functions is target-specific.  For our PTX
backend, for example, kernel functions are marked using a specific
calling convention.  Have a look at how I added PTX support for CUDA
kernel functions here: [1]

> tagging with metadata the alloca instructions for __local variables

The way I planned to handle this was to give __local variables with
function scope a static storage-class, so they would be codegen'd into
global variables rather than alloca instructions.  This makes the
implementation easier on the LLVM side as we don't yet have support
for address space attributes on alloca instructions (remember that
pointers to __local variables must also be __local).  I implemented
a patch [2] for this, but decided to wait until the OpenCL semantic
support in Clang was more mature.

Thanks,
-- 
Peter

[1] http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20101125/b306471a/attachment-0006.bin
[2] http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20101018/035558.html