aaron.ballman added a comment.

I don't know how language extensions come about in CUDA or HIP -- is there an 
appropriate standards body (or something similar) that's aware of this 
extension and supports it?

The changes should likely come with a release note entry about the new 
functionality, and some documentation changes as well.



================
Comment at: clang/include/clang/Basic/Attr.td:1778-1779
 def NoInline : DeclOrStmtAttr {
-  let Spellings = [GCC<"noinline">, CXX11<"clang", "noinline">,
+  let Spellings = [Keyword<"__noinline__">, GCC<"noinline">, CXX11<"clang", 
"noinline">,
                    C2x<"clang", "noinline">, Declspec<"noinline">];
   let Accessors = [Accessor<"isClangNoInline", [CXX11<"clang", "noinline">,
----------------



================
Comment at: clang/include/clang/Basic/Features.def:274
+// CUDA/HIP Features
+FEATURE(cuda_noinline_keyword, true)
+
----------------
Do the CUDA or HIP specs define `__noinline__` as a keyword specifically? If 
not, this isn't a `FEATURE`, it's an `EXTENSION` because it's specific to 
Clang, not the language standard.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:902
+  while (Tok.is(tok::kw___noinline__)) {
+    IdentifierInfo *AttrName = Tok.getIdentifierInfo();
+    SourceLocation AttrNameLoc = ConsumeToken();
----------------
I think we should we be issuing a pedantic "this is a clang extension" warning 
here, WDYT?


================
Comment at: clang/test/SemaCUDA/noinline.cu:8
+__attribute__((noinline)) void fun2() { }
+__attribute__((__noinline__)) void fun3() { }
----------------
I think there should also be a test like:
```
[[gnu::__noinline__]] void fun4() {}
```
to verify that the double square bracket syntax also correctly handles this 
being a keyword now (I expect the test to pass).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124866/new/

https://reviews.llvm.org/D124866

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to