[LLVMdev] [PATCH] OpenCL support - update on keywords

[Villmow, Micah]

Anton,
  Would there be any issue with switching the ordering of constant and local?

> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu]
> On Behalf Of Anton Lokhmotov
> Sent: Friday, February 18, 2011 7:27 AM
> To: 'Peter Collingbourne'
> Cc: cfe-dev at cs.uiuc.edu; llvmdev at cs.uiuc.edu
> Subject: [LLVMdev] [PATCH] OpenCL support - update on keywords
> 
> > -----Original Message-----
> > From: Peter Collingbourne [mailto:peter at pcc.me.uk]
> > Sent: 04 January 2011 21:42
> > To: Anton Lokhmotov
> > Cc: cfe-dev at cs.uiuc.edu
> > Subject: Re: OpenCL support
> >
> > Here are my comments on the second patch.
> >
> > > +enum OpenCLAddressSpace {
> > > +  OPENCL_PRIVATE = 0,
> > > +  OPENCL_GLOBAL = 1,
> > > +  OPENCL_LOCAL = 2,
> > > +  OPENCL_CONSTANT = 3
> > > +};
> >
> > If we are going to standardise these address space numbers across
> > Clang and the LLVM backends, this enum declaration should be added
> > somewhere in LLVM so that the backends have access to it.  Perhaps a
> > new header file called something like "llvm/Support/AddressSpaces.h"?
> We went for llvm/Support/OpenCL.h for now, because might need to add
> further OpenCL-specific support.  Copying to llvmdev.
> 
> > > +enum OpenCLImageAccess {
> > > +  OPENCL_READ_ONLY = 1,
> > > +  OPENCL_WRITE_ONLY = 2,
> > > +  OPENCL_READ_WRITE = 3
> > > +};
> >
> > This should live in AST somewhere - once the image access attribute
> is
> > added to the AST we will need access to this declaration from all AST
> > clients.
> Similarly, we created clang/AST/OpenCL.h.
> 
> > Also, please add test cases which test that:
> >
> > 1) the LLVM produced by the code generator is annotated with the
> >    correct address space numbers for each new address space added;
> Done (test/CodeGenOpenCL/address-spaces.cl).
> 
> > 2) the image access attributes are recognised by the parser/semantic
> >    analyser.
> Partially done (test/Parser/opencl-image-access.cl).  Semantic support
> for the image access qualifiers will be added along with support for
> the image types, which we are preparing for review now.
> 
> > Other than that, LGTM.
> 
> Many thanks,
> Anton.