[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

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

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

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

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

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

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

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

[yaxunl]

include/clang/Basic/Attr.td

701

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

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

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

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

12

Separate with an empty line here.

[yaxunl]

Insert an empty line to the test for clarity.

include/clang/Basic/Attr.td

 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

   let Documentation = [OpenCLAddressSpaceGenericDocs];
 }
 
+def OpenCLNoSVM : Attr {
+  let Spellings = [GNU<"nosvm">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [OpenCLNoSVMDocs];
+  let LangOpts = [OpenCL];
+}
+
 def Deprecated : InheritableAttr {
   let Spellings = [GCC<"deprecated">, Declspec<"deprecated">,
                    CXX11<"","deprecated", 201309>];

include/clang/Basic/AttrDocs.td

   }];
 }
 
+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``). 

test/SemaOpenCL/nosvm.cl

+// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -verify -x c -D NOT_OPENCL %s
+
+#ifndef NOT_OPENCL
+kernel void f(__attribute__((nosvm)) global int* a);
+__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