llvm dev - Feb 2011 - [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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 00002-keywords-llvm.patch
Type: application/octet-stream
Size: 875 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110218/86d3c7a6/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 00002-keywords-clang.patch
Type: application/octet-stream
Size: 16147 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110218/86d3c7a6/attachment-0001.obj>

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.

> > > > +enum OpenCLAddressSpace {
> > > > +  OPENCL_PRIVATE = 0,
> > > > +  OPENCL_GLOBAL = 1,
> > > > +  OPENCL_LOCAL = 2,
> > > > +  OPENCL_CONSTANT = 3
> > > > +};
> -----Original Message-----
> From: Villmow, Micah [mailto:Micah.Villmow at amd.com]
> 
> Anton,
>   Would there be any issue with switching the ordering of constant and
> local?
Hi Micah,

We'd rather not do that.  First, this order follows the order of subsections
in 6.5 of the OpenCL specification, with global described in 6.5.1, local in
6.5.2, constant in 6.5.3; private is described in 6.5.4 but it's also the
default function-scope space which is 0.  Second, we use the same order in
our (non-LLVM) GPU backend. 

Best regards,
Anton.

On Fri, Feb 18, 2011 at 03:27:13PM -0000, Anton Lokhmotov
wrote:
> > -----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.
I don't think we should include the word OpenCL in the name of this
enum/header.  We may very well in the future want to define address
space constants for other languages, which should not overlap with
the OpenCL constants, by extending this enum.

IMHO we shouldn't pollute the llvm namespace with this enum either.
In a similar style to the llvm/CallingConv.h header, we could
consider creating another namespace and placing the enum inside
(llvm::AddrSpace::ID perhaps?)

A few additional comments:

"private" should not be marked as a type qualifier, type specifier
qualifier or declaration specifier in languages other than OpenCL.
Otherwise we will end up accepting invalid C++ declarations such as:

private int i;

or even

int private i;

There are several 80-col violations in lib/Parse/ParseDecl.cpp
and in the CreateIntegerAttribute function.  git also picked
up some trailing whitespace errors.

To avoid the code duplication between ParseDeclarationSpecifiers,
ParseOptionalTypeSpecifier and ParseTypeQualifierListOpt you
should introduce a ParseOpenCLTypeAttributes function (e.g. see
ParseBorlandTypeAttributes).

Other than that, LGTM.

Thanks,
-- 
Peter

Hi Peter,

Many thanks for your prompt review.
> I don't think we should include the word OpenCL in the name of this
> enum/header.  We may very well in the future want to define address
> space constants for other languages, which should not overlap with
> the OpenCL constants, by extending this enum.
Good point.  We have defined in llvm/Support/AddressSpaces.h: 

+namespace llvm {
+  namespace AddressSpaces {
+    enum AddressSpace {
+      PRIVATE = 0,
+      GLOBAL = 1,
+      LOCAL = 2,
+      CONSTANT = 3
+    };
+  }
+}

(We were not quite sure whether to keep OPENCL_PRIVATE, etc., so removed the
prefix as well.  Feel free to put it back if needed.)
> "private" should not be marked as a type qualifier, type
specifier
> qualifier or declaration specifier in languages other than OpenCL.
Fully agree.  Should be fixed now.
> There are several 80-col violations in lib/Parse/ParseDecl.cpp
> and in the CreateIntegerAttribute function.  git also picked
> up some trailing whitespace errors.
Should be fixed now.
> To avoid the code duplication between ParseDeclarationSpecifiers,
> ParseOptionalTypeSpecifier and ParseTypeQualifierListOpt you
> should introduce a ParseOpenCLTypeAttributes function (e.g. see
> ParseBorlandTypeAttributes).
Done.
> Other than that, LGTM.
Thanks,
Anton.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 00002-keywords-llvm.patch
Type: application/octet-stream
Size: 935 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110224/92f609a1/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 00002-keywords-clang.patch
Type: application/octet-stream
Size: 15628 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110224/92f609a1/attachment-0001.obj>

On Thu, Feb 24, 2011 at 1:14 PM, Anton Lokhmotov
<Anton.Lokhmotov at arm.com> wrote:
> Hi Peter,
>
> Many thanks for your prompt review.
>
>> I don't think we should include the word OpenCL in the name of this
>> enum/header.  We may very well in the future want to define address
>> space constants for other languages, which should not overlap with
>> the OpenCL constants, by extending this enum.
> Good point.  We have defined in llvm/Support/AddressSpaces.h:
>
> +namespace llvm {
> +  namespace AddressSpaces {
> +    enum AddressSpace {
> +      PRIVATE = 0,
> +      GLOBAL = 1,
> +      LOCAL = 2,
> +      CONSTANT = 3
> +    };
> +  }
> +}
The address space mechanism is used by some code generators to
differentiate between physical memory spaces. The PIC16 code generator
uses address spaces 0 and 1 to select between its RAM and ROM spaces.
And X86 uses address space 256 for GS and 257 for FS. In the back end
for a dual-harvard DSP that I've been working on, I use address spaces
0-3 to designate the various memories on the machine.

The enum conflicts are easy enough to fix, but this current
implementation doesn't seem to leave room to specify both language-
and target-specific options on the same pointer. For example, when
developing an app for a PIC16, how would a user specify a pointer to a
CONSTANT variable in the ROM space?

Perhaps we could reserve separate bitfields within the address space
number for language- and target-specific options. The OpenCL code
would then need to shift and OR its constants with any address space
numbers specified with the __attribute__ syntax.

-Ken

On Fri, Feb 25, 2011 at 02:55:33PM -0500, Ken Dyck
wrote:
> The address space mechanism is used by some code generators to
> differentiate between physical memory spaces. The PIC16 code generator
> uses address spaces 0 and 1 to select between its RAM and ROM spaces.
> And X86 uses address space 256 for GS and 257 for FS. In the back end
> for a dual-harvard DSP that I've been working on, I use address spaces
> 0-3 to designate the various memories on the machine.
> 
> The enum conflicts are easy enough to fix, but this current
> implementation doesn't seem to leave room to specify both language-
> and target-specific options on the same pointer. For example, when
> developing an app for a PIC16, how would a user specify a pointer to a
> CONSTANT variable in the ROM space?
> 
> Perhaps we could reserve separate bitfields within the address space
> number for language- and target-specific options. The OpenCL code
> would then need to shift and OR its constants with any address space
> numbers specified with the __attribute__ syntax.
The more I think about it, the more I become uncomfortable with the
concept of language-specific address spaces in LLVM.  These are the
main issues I see with language-specific address spaces:

Firstly, it forces every target to 'know' about each source language,
requiring (potentially) modification of each target for each new
frontend language with multiple targets.  This goes against the LLVM
design principle of language independence, and encourages frontends
to reuse (abuse?) address spaces which are meant for other languages.

Secondly, consider the issue of language interoperability (e.g. a
hypothetical CUDA <-> OpenCL interop layer) -- we either lose the
ability to pass pointers between languages in a type-safe way or end
up giving awkward names to address spaces.

Instead of language-specific address spaces, each target should
concentrate on exposing all of its address spaces as target-specific
address spaces, and frontends should use a language -> target mapping
in target-specific code.  We can continue to expose the target's main
shared writable address space as address space 0 as we do now.

For example, Clang could define a set of internal address space
constants for OpenCL and use TargetCodeGenInfo to provide the mapping
to target address spaces.

An additional benefit is that this solution would allow AMD and
other backends with non-standard orderings [1] to retain backward
compatibility.

In Clang, by default, pointers would be in language address space 0,
which could map to any target address space (normally 0).  This neatly
resolves the "default address space" problem for devices with a
nonzero private address space (although on the LLVM side we would
need an address-space-aware alloca).

Thanks,
-- 
Peter

[1] http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-February/038199.html