Anton Lokhmotov
2011-Feb-18 15:27 UTC
[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>
Villmow, Micah
2011-Feb-18 19:44 UTC
[LLVMdev] [PATCH] OpenCL support - update on keywords
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.
Anton Lokhmotov
2011-Feb-21 09:45 UTC
[LLVMdev] [PATCH] OpenCL support - update on keywords
> > > > +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.
Peter Collingbourne
2011-Feb-23 19:26 UTC
[LLVMdev] [PATCH] OpenCL support - update on keywords
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
Anton Lokhmotov
2011-Feb-24 18:14 UTC
[LLVMdev] [PATCH] OpenCL support - update on keywords
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
Peter Collingbourne
2011-Feb-28 21:41 UTC
[LLVMdev] Language-specific vs target-specific address spaces (was Re: [PATCH] OpenCL support - update on keywords)
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
Reasonably Related Threads
- [LLVMdev] [PATCH] OpenCL support - update on keywords
- [LLVMdev] Language-specific vs target-specific address spaces (was Re: [PATCH] OpenCL support - update on keywords)
- [LLVMdev] [PATCH] OpenCL support - update on keywords
- [LLVMdev] [PATCH] OpenCL support - update on keywords
- [LLVMdev] Language-specific vs target-specific address spaces (was Re: [PATCH] OpenCL support - update on keywords)