[OpenCL] __generic address space for CL2.0
Aaron Ballman
aaron at aaronballman.com
Mon Nov 17 11:01:54 PST 2014
On Mon, Nov 17, 2014 at 1:21 PM, Anastasia Stulova
<anastasia.stulova at arm.com> wrote:
> Hi Aaron,
>
> Thanks for your comments! All issues are addressed in the attached update!
>
> -----Original Message-----
> From: aaron.ballman at gmail.com [mailto:aaron.ballman at gmail.com] On Behalf Of Aaron Ballman
>
>> These could be combined with a %select.
>
> I had to add a bit of extra code for this to work. Does it seem reasonable to you?
Thank you! I have some further comments, but they're mostly nitpicks
about the documentation. I hadn't realized how much work would be
involved with your change to use %select, thank you for doing it!
> diff --git a/include/clang/Basic/AddressSpaces.h b/include/clang/Basic/AddressSpaces.h
> index 4b1cea5..8dd7566 100644
> --- a/include/clang/Basic/AddressSpaces.h
> +++ b/include/clang/Basic/AddressSpaces.h
> @@ -30,6 +30,7 @@ enum ID {
> opencl_global = Offset,
> opencl_local,
> opencl_constant,
> + opencl_generic,
>
> cuda_device,
> cuda_constant,
> diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
> index 4fbbc66..f227df4 100644
> --- a/include/clang/Basic/Attr.td
> +++ b/include/clang/Basic/Attr.td
> @@ -622,22 +622,27 @@ def OpenCLImageAccess : Attr {
>
> def OpenCLPrivateAddressSpace : TypeAttr {
> let Spellings = [Keyword<"__private">, Keyword<"private">];
> - let Documentation = [Undocumented];
> + let Documentation = [OpenCLAddressSpacePrivateDocs];
> }
>
> def OpenCLGlobalAddressSpace : TypeAttr {
> let Spellings = [Keyword<"__global">, Keyword<"global">];
> - let Documentation = [Undocumented];
> + let Documentation = [OpenCLAddressSpaceGlobalDocs];
> }
>
> def OpenCLLocalAddressSpace : TypeAttr {
> let Spellings = [Keyword<"__local">, Keyword<"local">];
> - let Documentation = [Undocumented];
> + let Documentation = [OpenCLAddressSpaceLocalDocs];
> }
>
> def OpenCLConstantAddressSpace : TypeAttr {
> let Spellings = [Keyword<"__constant">, Keyword<"constant">];
> - let Documentation = [Undocumented];
> + let Documentation = [OpenCLAddressSpaceConstantDocs];
> +}
> +
> +def OpenCLGenericAddressSpace : TypeAttr {
> + let Spellings = [Keyword<"__generic">, Keyword<"generic">];
> + let Documentation = [OpenCLAddressSpaceGenericDocs];
> }
>
> def Deprecated : InheritableAttr {
> diff --git a/include/clang/Basic/AttrDocs.td b/include/clang/Basic/AttrDocs.td
> index cf8b662..5a1d55f 100644
> --- a/include/clang/Basic/AttrDocs.td
> +++ b/include/clang/Basic/AttrDocs.td
> @@ -1265,3 +1265,83 @@ for further details including limitations of the unroll hints.
> }];
> }
>
> +def DocOpenCLAddressSpaces : DocumentationCategory<"OpenCL Address Spaces"> {
Extra space in the string literal.
> + let Content = [{
> +The address space qualifier may be used in variable declarations to specify the
> +region of memory that is used to allocate the object. OpenCL supports the
> +following address spaces: __generic/generic, __global/global, __local/local,
> +__private/private, __constant/constant
> +
> + .. code-block:: opencl
> +
> + __constant int c = ...;
> +
> + __generic int* foo(global int* g) {
> + __local int* l;
> + private int p;
> + ...
> + return l;
> + }
> +
> + }];
> +}
> +
I thought that these were all type attributes, not a declaration
attributes? If so, the documentation is a bit confusing, because it
mostly focuses on declarations instead of types.
> +def OpenCLAddressSpaceGenericDocs : Documentation {
> + let Category = DocOpenCLAddressSpaces;
> + let Heading = "__generic/generic";
Instead of /, can you surround in parenthesis? That's how the other
attributes wind up being documented. I would recommend that whatever
form is preferred be the first form listed. e.g., generic (__generic).
Same comments apply to the other documentation headings.
> + let Content = [{
> +Generic address space is only available from OpenCL v2.0 and can be used with
> +a pointer type declarations either inside a function body or as a non-kernel
> +function arguments.
Some grammatical issues. Perhaps:
The generic address space attribute is only available with OpenCL v2.0
and later. It can be used with pointer type declarations for local
variables, or as function parameters in non-kernel functions.
> It is intended to be a place holder for any other address
> +space except '__constant' in a source code where multiple address spaces can be
> +used.
placeholder instead of place holder. Also, "a source code" is ungrammatical.
> + }];
> +}
> +
> +def OpenCLAddressSpaceConstantDocs : Documentation {
> + let Category = DocOpenCLAddressSpaces;
> + let Heading = "__constant/constant";
> + let Content = [{
> +Constant address space signals that an object is located in a constant
"The constant address space" ?
> +(non-modifiable) memory region. It is available to all work items.
> +Constant declarations are allowed with any type and in any scope. Such
"Types annotated with the constant address space attribute" instead of
"constant declarations" ?
> +variable declarations must have an initializer.
> + }];
> +}
> +
> +def OpenCLAddressSpaceGlobalDocs : Documentation {
> + let Category = DocOpenCLAddressSpaces;
> + let Heading = "__global/global";
> + let Content = [{
> +Global address space
Same suggestion here as above.
> specifies that an object is allocated in the global memory,
Perhaps remove "the" from "the global memory?"
> +which is accessible by all work items. The content stored in this memory
> +area persists between kernel executions. Variables declared in global address
"in the global address" ?
> +space are allowed as function parameters or inside a function body (and must be
> +pointer types in both cases). Starting from OpenCL v2.0, global address space can
> +be used with program scope variables as well.
Instead of function bodies and program scope, perhaps local and global
scope instead?
> + }];
> +}
> +
> +def OpenCLAddressSpaceLocalDocs : Documentation {
> + let Category = DocOpenCLAddressSpaces;
> + let Heading = "__local/local";
> + let Content = [{
> +Local address space
Same suggestion here as above.
> specifies that an object is allocated in the local to a work
I am not certain what "in the local to a work" means.
> +group memory area, which is accessible by all work items in the same work group.
> +The content stored in this memory region is not accessible after the kernel
> +execution ends. Variables declared in local address space are allowed as function
"in the local address space" ?
> +parameters or inside a kernel function body.
> + }];
> +}
> +
> +def OpenCLAddressSpacePrivateDocs : Documentation {
> + let Category = DocOpenCLAddressSpaces;
> + let Heading = "__private/private";
> + let Content = [{
> +Private address space
Same suggestion here as above.
> specifies that an object is allocated in the private to
> +a work item memory.
I am not certain what "in the private to a work item" means.
> Other work items can not access the same memory area and its
Cannot instead of can not (isn't English wonderful?).
> +content is destroyed after work item execution ends. Declarations in
> +private address space are allowed as non-kernel function parameters or inside
"in the private address space" instead?
> +a function body.
Local scope instead?
> + }];
> +}
> diff --git a/include/clang/Basic/DiagnosticParseKinds.td b/include/clang/Basic/DiagnosticParseKinds.td
> index eb03f5a..c69a541 100644
> --- a/include/clang/Basic/DiagnosticParseKinds.td
> +++ b/include/clang/Basic/DiagnosticParseKinds.td
> @@ -932,8 +932,8 @@ def err_pragma_optimize_extra_argument : Error<
> "unexpected extra argument '%0' to '#pragma clang optimize'">;
>
> // OpenCL Section 6.8.g
> -def err_not_opencl_storage_class_specifier : Error<
> - "OpenCL does not support the '%0' storage class specifier">;
> +def err_opencl_unknown_type_specifier : Error<
> + "OpenCL does not support the '%0' %select{type qualifier|storage class specifier}1">;
>
> // OpenCL EXTENSION pragma (OpenCL 1.1 [9.1])
> def warn_pragma_expected_colon : Warning<
> diff --git a/include/clang/Basic/TokenKinds.def b/include/clang/Basic/TokenKinds.def
> index c96b8eb..8b25e81 100644
> --- a/include/clang/Basic/TokenKinds.def
> +++ b/include/clang/Basic/TokenKinds.def
> @@ -470,10 +470,12 @@ KEYWORD(__global , KEYOPENCL)
> KEYWORD(__local , KEYOPENCL)
> KEYWORD(__constant , KEYOPENCL)
> KEYWORD(__private , KEYOPENCL)
> +KEYWORD(__generic , KEYOPENCL)
> ALIAS("global", __global , KEYOPENCL)
> ALIAS("local", __local , KEYOPENCL)
> ALIAS("constant", __constant , KEYOPENCL)
> ALIAS("private", __private , KEYOPENCL)
> +ALIAS("generic", __generic , KEYOPENCL)
> // OpenCL function qualifiers
> KEYWORD(__kernel , KEYOPENCL)
> ALIAS("kernel", __kernel , KEYOPENCL)
> diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
> index ae72c58..6554a49 100644
> --- a/lib/AST/ASTContext.cpp
> +++ b/lib/AST/ASTContext.cpp
> @@ -699,9 +699,10 @@ static const LangAS::Map *getAddressSpaceMap(const TargetInfo &T,
> 1, // opencl_global
> 2, // opencl_local
> 3, // opencl_constant
> - 4, // cuda_device
> - 5, // cuda_constant
> - 6 // cuda_shared
> + 4, // opencl_generic
> + 5, // cuda_device
> + 6, // cuda_constant
> + 7 // cuda_shared
> };
> return &FakeAddrSpaceMap;
> } else {
> diff --git a/lib/AST/TypePrinter.cpp b/lib/AST/TypePrinter.cpp
> index f36e799..e36fc17 100644
> --- a/lib/AST/TypePrinter.cpp
> +++ b/lib/AST/TypePrinter.cpp
> @@ -1490,6 +1490,9 @@ void Qualifiers::print(raw_ostream &OS, const PrintingPolicy& Policy,
> case LangAS::opencl_constant:
> OS << "__constant";
> break;
> + case LangAS::opencl_generic:
> + OS << "__generic";
> + break;
> default:
> OS << "__attribute__((address_space(";
> OS << addrspace;
> diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp
> index 61f0a53..44842f6 100644
> --- a/lib/Basic/Targets.cpp
> +++ b/lib/Basic/Targets.cpp
> @@ -1367,6 +1367,8 @@ namespace {
> 1, // opencl_global
> 3, // opencl_local
> 4, // opencl_constant
> + // FIXME: generic has to be added to the target
> + 0, // opencl_generic
> 1, // cuda_device
> 4, // cuda_constant
> 3, // cuda_shared
> @@ -1485,6 +1487,8 @@ static const unsigned R600AddrSpaceMap[] = {
> 1, // opencl_global
> 3, // opencl_local
> 2, // opencl_constant
> + // FIXME: generic has to be added to the target
> + 0, // opencl_generic
> 1, // cuda_device
> 2, // cuda_constant
> 3 // cuda_shared
> @@ -5358,6 +5362,8 @@ namespace {
> 3, // opencl_global
> 4, // opencl_local
> 5, // opencl_constant
> + // FIXME: generic has to be added to the target
> + 0, // opencl_generic
> 0, // cuda_device
> 0, // cuda_constant
> 0 // cuda_shared
> @@ -6082,6 +6088,7 @@ namespace {
> 1, // opencl_global
> 3, // opencl_local
> 2, // opencl_constant
> + 4, // opencl_generic
This is missing a FIXME, is that intentional?
> 0, // cuda_device
> 0, // cuda_constant
> 0 // cuda_shared
> diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
> index 66b98c2..7ddbab0 100644
> --- a/lib/Parse/ParseDecl.cpp
> +++ b/lib/Parse/ParseDecl.cpp
> @@ -2519,6 +2519,7 @@ void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
> const PrintingPolicy &Policy = Actions.getASTContext().getPrintingPolicy();
> while (1) {
> bool isInvalid = false;
> + bool isStorageClass =false;
Missing a space after the =.
> const char *PrevSpec = nullptr;
> unsigned DiagID = 0;
>
> @@ -2942,22 +2943,26 @@ void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
> case tok::kw_typedef:
> isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_typedef, Loc,
> PrevSpec, DiagID, Policy);
> + isStorageClass = true;
> break;
> case tok::kw_extern:
> if (DS.getThreadStorageClassSpec() == DeclSpec::TSCS___thread)
> Diag(Tok, diag::ext_thread_before) << "extern";
> isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_extern, Loc,
> PrevSpec, DiagID, Policy);
> + isStorageClass = true;
> break;
> case tok::kw___private_extern__:
> isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_private_extern,
> Loc, PrevSpec, DiagID, Policy);
> + isStorageClass = true;
> break;
> case tok::kw_static:
> if (DS.getThreadStorageClassSpec() == DeclSpec::TSCS___thread)
> Diag(Tok, diag::ext_thread_before) << "static";
> isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_static, Loc,
> PrevSpec, DiagID, Policy);
> + isStorageClass = true;
> break;
> case tok::kw_auto:
> if (getLangOpts().CPlusPlus11) {
> @@ -2973,18 +2978,22 @@ void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
> } else
> isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_auto, Loc,
> PrevSpec, DiagID, Policy);
> + isStorageClass = true;
> break;
> case tok::kw_register:
> isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_register, Loc,
> PrevSpec, DiagID, Policy);
> + isStorageClass = true;
> break;
> case tok::kw_mutable:
> isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_mutable, Loc,
> PrevSpec, DiagID, Policy);
> + isStorageClass = true;
> break;
> case tok::kw___thread:
> isInvalid = DS.SetStorageClassSpecThread(DeclSpec::TSCS___thread, Loc,
> PrevSpec, DiagID);
> + isStorageClass = true;
> break;
> case tok::kw_thread_local:
> isInvalid = DS.SetStorageClassSpecThread(DeclSpec::TSCS_thread_local, Loc,
> @@ -2993,6 +3002,7 @@ void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
> case tok::kw__Thread_local:
> isInvalid = DS.SetStorageClassSpecThread(DeclSpec::TSCS__Thread_local,
> Loc, PrevSpec, DiagID);
> + isStorageClass = true;
> break;
>
> // function-specifier
> @@ -3231,6 +3241,15 @@ void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
> break;
>
> // OpenCL qualifiers:
> + case tok::kw___generic:
> + // generic address space is introduced only in OpenCL v2.0
> + // see OpenCL C Spec v2.0 s6.5.5
> + if (Actions.getLangOpts().OpenCLVersion < 200) {
> + DiagID = diag::err_opencl_unknown_type_specifier;
> + PrevSpec = Tok.getIdentifierInfo()->getNameStart();
> + isInvalid = true;
> + break;
> + };
> case tok::kw___private:
> case tok::kw___global:
> case tok::kw___local:
> @@ -3265,6 +3284,8 @@ void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
> if (DiagID == diag::ext_duplicate_declspec)
> Diag(Tok, DiagID)
> << PrevSpec << FixItHint::CreateRemoval(Tok.getLocation());
> + else if (DiagID == diag::err_opencl_unknown_type_specifier)
> + Diag(Tok, DiagID) << PrevSpec << isStorageClass;
> else
> Diag(Tok, DiagID) << PrevSpec;
> }
> @@ -3981,6 +4002,7 @@ bool Parser::isTypeQualifier() const {
> case tok::kw___local:
> case tok::kw___global:
> case tok::kw___constant:
> + case tok::kw___generic:
> case tok::kw___read_only:
> case tok::kw___read_write:
> case tok::kw___write_only:
> @@ -4130,6 +4152,7 @@ bool Parser::isTypeSpecifierQualifier() {
> case tok::kw___local:
> case tok::kw___global:
> case tok::kw___constant:
> + case tok::kw___generic:
> case tok::kw___read_only:
> case tok::kw___read_write:
> case tok::kw___write_only:
> @@ -4302,6 +4325,7 @@ bool Parser::isDeclarationSpecifier(bool DisambiguatingWithExpression) {
> case tok::kw___local:
> case tok::kw___global:
> case tok::kw___constant:
> + case tok::kw___generic:
> case tok::kw___read_only:
> case tok::kw___read_write:
> case tok::kw___write_only:
> @@ -4490,6 +4514,7 @@ void Parser::ParseTypeQualifierListOpt(DeclSpec &DS, unsigned AttrReqs,
> case tok::kw___global:
> case tok::kw___local:
> case tok::kw___constant:
> + case tok::kw___generic:
> case tok::kw___read_only:
> case tok::kw___write_only:
> case tok::kw___read_write:
> diff --git a/lib/Sema/DeclSpec.cpp b/lib/Sema/DeclSpec.cpp
> index 6e77d27..a9fe9b6 100644
> --- a/lib/Sema/DeclSpec.cpp
> +++ b/lib/Sema/DeclSpec.cpp
> @@ -500,14 +500,14 @@ bool DeclSpec::SetStorageClassSpec(Sema &S, SCS SC, SourceLocation Loc,
> case SCS_private_extern:
> case SCS_static:
> if (S.getLangOpts().OpenCLVersion < 120) {
> - DiagID = diag::err_not_opencl_storage_class_specifier;
> + DiagID = diag::err_opencl_unknown_type_specifier;
> PrevSpec = getSpecifierName(SC);
> return true;
> }
> break;
> case SCS_auto:
> case SCS_register:
> - DiagID = diag::err_not_opencl_storage_class_specifier;
> + DiagID = diag::err_opencl_unknown_type_specifier;
> PrevSpec = getSpecifierName(SC);
> return true;
> default:
> diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
> index a6f9483..688150a 100644
> --- a/lib/Sema/SemaType.cpp
> +++ b/lib/Sema/SemaType.cpp
> @@ -3980,6 +3980,8 @@ static void HandleAddressSpaceTypeAttribute(QualType &Type,
> ASIdx = LangAS::opencl_local; break;
> case AttributeList::AT_OpenCLConstantAddressSpace:
> ASIdx = LangAS::opencl_constant; break;
> + case AttributeList::AT_OpenCLGenericAddressSpace:
> + ASIdx = LangAS::opencl_generic; break;
> default:
> assert(Attr.getKind() == AttributeList::AT_OpenCLPrivateAddressSpace);
> ASIdx = 0; break;
> @@ -4891,6 +4893,7 @@ static void processTypeAttrs(TypeProcessingState &state, QualType &type,
> case AttributeList::AT_OpenCLGlobalAddressSpace:
> case AttributeList::AT_OpenCLLocalAddressSpace:
> case AttributeList::AT_OpenCLConstantAddressSpace:
> + case AttributeList::AT_OpenCLGenericAddressSpace:
> case AttributeList::AT_AddressSpace:
> HandleAddressSpaceTypeAttribute(type, attr, state.getSema());
> attr.setUsedAsTypeAttr();
> diff --git a/test/Parser/opencl-cl20.cl b/test/Parser/opencl-cl20.cl
> new file mode 100644
> index 0000000..b718699
> --- /dev/null
> +++ b/test/Parser/opencl-cl20.cl
> @@ -0,0 +1,26 @@
> +// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
> +// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0 -DCL20
> +
> +#ifdef CL20
> +// expected-no-diagnostics
> +#endif
> +
> +__generic int * __generic_test(__generic int *arg) {
> + __generic int *var;
> + return var;
> +}
> +#ifndef CL20
> +// expected-error at -5 {{OpenCL does not support the '__generic' type qualifier}}
> +// expected-error at -6 {{OpenCL does not support the '__generic' type qualifier}}
> +// expected-error at -6 {{OpenCL does not support the '__generic' type qualifier}}
> +#endif
> +
> +generic int * generic_test(generic int *arg) {
> + generic int *var;
> + return var;
> +}
> +#ifndef CL20
> +// expected-error at -5 {{OpenCL does not support the 'generic' type qualifier}}
> +// expected-error at -6 {{OpenCL does not support the 'generic' type qualifier}}
> +// expected-error at -6 {{OpenCL does not support the 'generic' type qualifier}}
> +#endif
>
~Aaron
More information about the cfe-commits
mailing list