[PATCH] D17861: [OpenCL] Accept __attribute__((nosvm))

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: test/SemaOpenCL/nosvm.cl:9
@@ +8,3 @@
+#else
+void f(__attribute__((nosvm)) int* a); // expected-warning {{'nosvm' attribute ignored}}
+#endif
----------------
yaxunl wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > It seems like it makes nosvm available for C and earlier standards of OpenCL too?
> > > > > 
> > > > > Doesn't feel right. :(
> > > > 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?
> > > 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.
> > 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. 
> 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
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!


Repository:
  rL LLVM

http://reviews.llvm.org/D17861