[OpenCL] Accept __attribute__((nosvm))

[yaxunl]

Summary

OpenCL 2.0 supports attribute((nosvm)) but it is not widely used and removed from OpenCL 2.1.

This change allows Clang to accept attribute((nosvm)) for OpenCL but ignores it.

Differential Revision: http://reviews.llvm.org/D17861

[Anastasia]

test/SemaOpenCL/nosvm.cl

9 ↗

(On Diff #49763)

It seems like it makes nosvm available for C and earlier standards of OpenCL too?

Doesn't feel right. :(

[yaxunl]

test/SemaOpenCL/nosvm.cl

9 ↗

(On Diff #49763)

Before my change, the original behavior of Clang is to emits a warning for any unknown GNU attribute, e.g.
unknown 'nosvm' attribute is ignored.

Should I just emit the same warning if the language is not OpenCL or if the language is OpenCL but the version is not 2.0? Or I should emit an error if the language is OpenCL but the version is not 2.0?

[yaxunl]

test/SemaOpenCL/nosvm.cl

9 ↗

(On Diff #49763)

Also, `'nosvm' attribute ignored` is the default warning msg used in tablegen when LangOpt<"OpenCL"> is used. The following code is generated by tablegen:

static bool checkOpenCLLangOpts(Sema &S, const AttributeList &Attr) {
  if (S.LangOpts.OpenCL)
    return true;

  S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName();
  return false;
}

Basically, if an attrib is defined for a specific language by using LangOpt<> in Attr.td, when this attr is used in other languages, the original behavior of Clang is to emit a warning like:

'nosvm' attribute ignored

Do we want to fix this? Or simply don't use LangOpt<> for this attribute.

[Anastasia]

test/SemaOpenCL/nosvm.cl

9 ↗

(On Diff #49763)

I am a bit confused now. So what diagnostic would Clang give if we don't use LanOpt<>? Is it the same as if with LangOpt<>? Is there anything we can do about CL version as well?

I don't feel like we should modify default Clang behavior wrt attributes a lot. Let's try to find the best way to use what we already have.

[yaxunl]

test/SemaOpenCL/nosvm.cl

9 ↗

(On Diff #49763)

For non-OpenCL programs,

If we don't use LangOpt, Clang will emit warning

unknown 'nosvm' attribute ignored

If we use LangOpt, Clang will emit warning

'nosvm' attribute ignored

If we don't want to change Clang default behavior but also don't like the second default warning, we should not use LangOpt.

I can emit a warning for OpenCL program which version is not 2.0:

'nosvm' attribute is valid only for OpenCL 2.0

and emit a warning for non-OpenCL programs

unknown 'nosvm' attribute is ignored

[Anastasia]

Please, add cfe-commit to Subscribers!

[Anastasia]

test/SemaOpenCL/nosvm.cl

9 ↗

(On Diff #49763)

Yes, seems like it's best not to use LangOpt actually.

So if the change is not large I would prefer the second option you proposed!

[aaron.ballman]

include/clang/Basic/Attr.td

701 ↗

(On Diff #49763)

Since the attribute is ignored by clang, you should inherit from IgnoredAttr.

[yaxunl]

include/clang/Basic/Attr.td

701 ↗

(On Diff #49763)

I tried that from beginning. If I inherit from IgnoredAttr, it seems to be ignored by the parser and won't reach the sema check part, and I cannot emit error msg based on OpenCL version.

[aaron.ballman]

include/clang/Basic/Attr.td

701 ↗

(On Diff #49763)

Ah. so it isn't *totally* ignored. Okay, in that case, you should set ASTNode = 0 and SemaHandler = 0.

[yaxunl]

include/clang/Basic/Attr.td

701 ↗

(On Diff #49763)

If I set ASTNode = 0 and SemaHandler = 0, tablegen will not generate an id AT_OpenCLNoSVM to identify this attribute, and I cannot diagnose its invalid usage in SemaDeclAttr.cpp due to that. Then it seems the only viable place to diagnose its invalid usage is in Parser::ParseGNUAttributes. However currently Parser::ParseGNUAttributes only does generic processing of GNU attributes and does not diagnose specific attribute. Since there is no id for NoSVM attribute, I have to diagnose based on its name string.

Do we really want to do this?

[aaron.ballman]

include/clang/Basic/Attr.td

701 ↗

(On Diff #49763)

Oh! I forgot that setting SemaHandler to 0 means that the AttributeList enumeration won't get an entry for the attribute, so we probably diagnose it as being unknown rather than getting to the language option checking. Ugh, sorry for the noise, you will want SemaHandler = 1 still (which it is by default, so no need to explicitly specify that), but ASTNode = 0 since we don't generate any AST information for the attribute.

[yaxunl]

Revised by Aaron's and Anastasia's comments.

Changed error/warning message for invalid usage of nosvm attribute.

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

5210 ↗

(On Diff #51821)

Please use the language option feature in tabelgen as in your previous patch instead of performing this work manually. We want use of the attribute to provide a different diagnostic in this case because the attribute *isn't* unknown, it simply doesn't apply under the compilation target (and hence is ignored).

5218 ↗

(On Diff #51821)

This should have an else clause telling the user that the attribute is being ignored; silently accepting it and then not performing the semantics of the attribute will be confusing behavior for most users. It might not be a bad idea to also have the diagnostic tell the user that the attribute has been deprecated and is removed in OpenCL 2.1.

[yaxunl]

Revised as Aaron suggested.

[aaron.ballman]

LGTM, thank you!

[Anastasia]

LGTM, except for one tiny comment that can be addressed before committing.

test/SemaOpenCL/nosvm.cl

11 ↗

(On Diff #51967)

Separate with an empty line here.

[yaxunl]

Insert an empty line to the test for clarity.

cfe/trunk/include/clang/Basic/Attr.td

 class LangOpt<string name, bit negated = 0> {
   string Name = name;
   bit Negated = negated;
 }
 def MicrosoftExt : LangOpt<"MicrosoftExt">;
 def Borland : LangOpt<"Borland">;
 def CUDA : LangOpt<"CUDA">;
 def COnly : LangOpt<"CPlusPlus", 1>;
+def OpenCL : LangOpt<"OpenCL">;
 
 // Defines targets for target-specific attributes. The list of strings should
 // specify architectures for which the target applies, based off the ArchType
 // enumeration in Triple.h.
 class TargetArch<list<string> arches> {
   list<string> Arches = arches;
   list<string> OSes;
   list<string> CXXABIs;
def OpenCLConstantAddressSpace : TypeAttr {
   let Documentation = [OpenCLAddressSpaceConstantDocs];
 }
 
 def OpenCLGenericAddressSpace : TypeAttr {
   let Spellings = [Keyword<"__generic">, Keyword<"generic">];
   let Documentation = [OpenCLAddressSpaceGenericDocs];
 }
 
+def OpenCLNoSVM : Attr {
+  let Spellings = [GNU<"nosvm">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [OpenCLNoSVMDocs];
+  let LangOpts = [OpenCL];
+  let ASTNode = 0;
+}
+
 def Deprecated : InheritableAttr {
   let Spellings = [GCC<"deprecated">, Declspec<"deprecated">,
                    CXX11<"","deprecated", 201309>];
   let Args = [StringArgument<"Message", 1>,
               // An optional string argument that enables us to provide a
               // Fix-It.
               StringArgument<"Replacement", 1>];
   let Documentation = [DeprecatedDocs];

cfe/trunk/include/clang/Basic/AttrDocs.td

 (work item) memory. Other work items cannot access the same memory area and its
 content is destroyed after work item execution ends. Local variables can be
 declared in the private address space. Function arguments are always in the
 private address space. Kernel function arguments of a pointer or an array type
 cannot point to the private address space.
   }];
 }
 
+def OpenCLNoSVMDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
+OpenCL 2.0 supports the optional ``__attribute__((nosvm))`` qualifier for
+pointer variable. It informs the compiler that the pointer does not refer
+to a shared virtual memory region. See OpenCL v2.0 s6.7.2 for details.
+
+Since it is not widely used and has been removed from OpenCL 2.1, it is ignored
+by Clang.
+  }];
+}
 def NullabilityDocs : DocumentationCategory<"Nullability Attributes"> {
   let Content = [{
 Whether a particular pointer may be "null" is an important concern when working with pointers in the C family of languages. The various nullability attributes indicate whether a particular pointer can be null or not, which makes APIs more expressive and can help static analysis tools identify bugs involving null pointers. Clang supports several kinds of nullability attributes: the ``nonnull`` and ``returns_nonnull`` attributes indicate which function or method parameters and result types can never be null, while nullability type qualifiers indicate which pointer types can be null (``_Nullable``) or cannot be null (``_Nonnull``). 
 
 The nullability (type) qualifiers express whether a value of a given pointer type can be null (the ``_Nullable`` qualifier), doesn't have a defined meaning for null (the ``_Nonnull`` qualifier), or for which the purpose of null is unclear (the ``_Null_unspecified`` qualifier). Because nullability qualifiers are expressed within the type system, they are more general than the ``nonnull`` and ``returns_nonnull`` attributes, allowing one to express (for example) a nullable pointer to an array of nonnull pointers. Nullability qualifiers are written to the right of the pointer to which they apply. For example:
 
   .. code-block:: c
 

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_too_few_arguments : Error<
   "%0 attribute takes at least %1 argument%s1">;
 def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;
 def err_attribute_bad_neon_vector_size : Error<
   "Neon vector size must be 64 or 128 bits">;
 def err_attribute_requires_positive_integer : Error<
   "%0 attribute requires a positive integral compile time constant expression">;
 def err_attribute_requires_opencl_version : Error<
-  "%0 attribute requires OpenCL version %1 or above">;
+  "%0 attribute requires OpenCL version %1%select{| or above}2">;
 def warn_unsupported_target_attribute
     : Warning<"Ignoring unsupported '%0' in the target attribute string">,
     InGroup<IgnoredAttributes>;
 def err_attribute_unsupported
     : Error<"%0 attribute is not supported for this target">;
 // The err_*_attribute_argument_not_int are seperate because they're used by
 // VerifyIntegerConstantExpression.
 def err_aligned_attribute_argument_not_int : Error<

 def err_opencl_invalid_type_array : Error<
   "array of %0 type is invalid in OpenCL">;
 def err_opencl_ternary_with_block : Error<
   "block type cannot be used as expression in ternary expression in OpenCL">;
 def err_opencl_pointer_to_type : Error<
   "pointer to type %0 is invalid in OpenCL">;
 def err_opencl_type_can_only_be_used_as_function_parameter : Error <
   "type %0 can only be used as a function parameter in OpenCL">;
+def warn_opencl_attr_deprecated_ignored : Warning <
+  "%0 attribute is deprecated and ignored in OpenCL version %1">,
+  InGroup<IgnoredAttributes>;
 
 // OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions
 def err_opencl_builtin_pipe_first_arg : Error<
   "first argument to %0 must be a pipe type">;
 def err_opencl_builtin_pipe_arg_num : Error<
   "invalid number of arguments to function: %0">;
 def err_opencl_builtin_pipe_invalid_arg : Error<
   "invalid argument type to function %0 (expecting %1 having %2)">;

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

 static void handleInternalLinkageAttr(Sema &S, Decl *D,
                                       const AttributeList &Attr) {
   if (InternalLinkageAttr *Internal =
           S.mergeInternalLinkageAttr(D, Attr.getRange(), Attr.getName(),
                                      Attr.getAttributeSpellingListIndex()))
     D->addAttr(Internal);
 }
 
+static void handleOpenCLNoSVMAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  if (S.LangOpts.OpenCLVersion != 200)
+    S.Diag(Attr.getLoc(), diag::err_attribute_requires_opencl_version)
+        << Attr.getName() << "2.0" << 0;
+  else
+    S.Diag(Attr.getLoc(), diag::warn_opencl_attr_deprecated_ignored)
+        << Attr.getName() << "2.0";
+}
+
 /// Handles semantic checking for features that are common to all attributes,
 /// such as checking whether a parameter was properly specified, or the correct
 /// number of arguments were passed, etc.
 static bool handleCommonAttributeFeatures(Sema &S, Scope *scope, Decl *D,
                                           const AttributeList &Attr) {
   // Several attributes carry different semantics than the parsing requires, so
   // those are opted out of the common handling.
   //
case AttributeList::AT_SwiftContext:
     handleParameterABIAttr(S, D, Attr, ParameterABI::SwiftContext);
     break;
   case AttributeList::AT_SwiftErrorResult:
     handleParameterABIAttr(S, D, Attr, ParameterABI::SwiftErrorResult);
     break;
   case AttributeList::AT_SwiftIndirectResult:
     handleParameterABIAttr(S, D, Attr, ParameterABI::SwiftIndirectResult);
     break;
+  case AttributeList::AT_OpenCLNoSVM:
+    handleOpenCLNoSVMAttr(S, D, Attr);
+    break;
   case AttributeList::AT_InternalLinkage:
     handleInternalLinkageAttr(S, D, Attr);
     break;
 
   // Microsoft attributes:
   case AttributeList::AT_MSNoVTable:
     handleSimpleAttribute<MSNoVTableAttr>(S, D, Attr);
     break;

cfe/trunk/lib/Sema/SemaStmtAttr.cpp

 static Attr *handleOpenCLUnrollHint(Sema &S, Stmt *St, const AttributeList &A,
                                     SourceRange Range) {
   // OpenCL v2.0 s6.11.5 - opencl_unroll_hint can have 0 arguments (compiler
   // determines unrolling factor) or 1 argument (the unroll factor provided
   // by the user).
 
   if (S.getLangOpts().OpenCLVersion < 200) {
     S.Diag(A.getLoc(), diag::err_attribute_requires_opencl_version)
-        << A.getName() << "2.0";
+        << A.getName() << "2.0" << 1;
     return nullptr;
   }
 
   unsigned NumArgs = A.getNumArgs();
 
   if (NumArgs > 1) {
     S.Diag(A.getLoc(), diag::err_attribute_too_many_arguments) << A.getName()
                                                                << 1;

cfe/trunk/test/SemaOpenCL/nosvm.cl

+// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -verify -cl-std=CL2.0 -D CL20 %s
+// RUN: %clang_cc1 -verify -x c -D NOCL %s
+
+#ifndef NOCL
+kernel void f(__attribute__((nosvm)) global int* a);
+#ifndef CL20
+// expected-error@-2 {{'nosvm' attribute requires OpenCL version 2.0}}
+#else
+// expected-warning@-4 {{'nosvm' attribute is deprecated and ignored in OpenCL version 2.0}}
+#endif
+
+__attribute__((nosvm)) void g(); // expected-warning {{'nosvm' attribute only applies to variables}}
+
+#else
+void f(__attribute__((nosvm)) int* a); // expected-warning {{'nosvm' attribute ignored}}
+#endif