yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
================
Comment at: clang/docs/ReleaseNotes.rst:344-345
-CUDA Language Changes in Clang
+CUDA/HIP Language Changes in Clang
------------------------------
----------------
aaron.ballman wrote:
>
will fix
================
Comment at: clang/include/clang/Basic/AttrDocs.td:543
+avoid diagnostics due to usage of ``__attribute__((__noinline__))``
+with ``__noinline__`` defined as a macro as ``__attribute__((noinline))`.
+
----------------
aaron.ballman wrote:
>
will fix.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:902
+ while (Tok.is(tok::kw___noinline__)) {
+ IdentifierInfo *AttrName = Tok.getIdentifierInfo();
+ SourceLocation AttrNameLoc = ConsumeToken();
----------------
tra wrote:
> yaxunl wrote:
> > tra wrote:
> > > aaron.ballman wrote:
> > > > yaxunl wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think we should we be issuing a pedantic "this is a clang
> > > > > > extension" warning here, WDYT?
> > > > > will do
> > > > I'm questioning whether my advice here was good or not -- now that I
> > > > see the CUDA spec already calls these function qualifiers... it's
> > > > debatable whether this is a Clang extension or just the way in which
> > > > Clang implements the CUDA function qualifiers. @tra -- do you have
> > > > opinions?
> > > >
> > > > I'm sort of leaning towards dropping the extension warning, but the
> > > > only reason I can think of for keeping it is if Clang is the only CUDA
> > > > compiler that doesn't require you to include a header before using the
> > > > function qualifiers. If that's the case, there is a portability concern.
> > > I'm not sure if such a warning would be useful.
> > >
> > > > the only reason I can think of for keeping it is if Clang is the only
> > > > CUDA compiler that doesn't require you to include a header before using
> > > > the function qualifiers. If that's the case, there is a portability
> > > > concern.
> > >
> > > I don't think it's an issue.
> > >
> > > We already have similar divergence between nvcc/clang. E.g. built-in
> > > variables like `threadIdx`. Clang implements them in a header, but NVCC
> > > provides them by compiler itself.
> > > With both compilers the variables are available by the time we get to
> > > compile user code. Virtually all CUDA compilations are done with tons of
> > > CUDA headers pre-included by compiler. Those that do not do that are
> > > already on their own and have to provide many other 'standard' CUDA
> > > things like target attributes. I don't think we need to worry about that.
> > >
> > I can remove the diagnostics since it seems unnecessary.
> >
> > I tend to treat it as an extension since nvcc is the de facto standard
> > implementation, which does not implement it as a keyword. Compared to that,
> > this is like an extension.
> I'd argue that NVCC does implement it (as in "documents and makes it
> available"). Providing the documented functionality using a different
> implementation does not reach the point of being an extension, IMO. While
> there are observable differences between implementations, depending on them
> would be a portability error for the user.
>
that makes sense. will change the extension to feature
================
Comment at: clang/test/SemaCUDA/noinline.cu:8
+__attribute__((noinline)) void fun2() { }
+__attribute__((__noinline__)) void fun3() { }
----------------
aaron.ballman wrote:
> yaxunl wrote:
> > aaron.ballman wrote:
> > > yaxunl wrote:
> > > > aaron.ballman wrote:
> > > > > 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).
> > > > will do
> > > Ah, I just noticed we also have no tests for the behavior of the keyword
> > > in the presence of the macro being defined. e.g.,
> > > ```
> > > #define __noinline__ __attribute__((__noinline__))
> > > __noinline__ void fun5() {}
> > > ```
> > will do
> I missed an important detail -- I think this is now going to generate a
> warning in `-pedantic` mode (through `-Wkeyword-macro`) when compiling for
> CUDA; is that going to be a problem for CUDA headers, or are those always
> included as a system header (and so the diagnostics will be suppressed)?
I could not find how clang driver adds CUDA include path
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Cuda.cpp#L284
@tra do you know how CUDA include path is added? is it done by CMake?
For HIP the HIP include path is added as a system include path by clang driver.
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
