[PATCH] D124866: [CUDA][HIP] support __noinline__ as keyword

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: erichkeane.
aaron.ballman added a comment.

In D124866#3501181 <https://reviews.llvm.org/D124866#3501181>, @yaxunl wrote:

> In D124866#3500761 <https://reviews.llvm.org/D124866#3500761>, @aaron.ballman wrote:
>
>> Should we do `__forceinline__` at the same time so that there's consistency?
>
> `__forceinline__` does not have the issue as `__noinline__` has since it is not a GCC attribute. The current CUDA/HIP implementation of `__forceinline__` in header files is sufficient. I do not see the benefit of implementing `__forceinline__` as a keyword.

Primarily to reduce user confusion. It's kind of weird for `__noinline__` to be a keyword and `__forceinline__` to not be a keyword when they're both defined the same way by the CUDA spec. This means you can #undef one of them but not the other, that sort of thing.



================
Comment at: clang/test/CodeGenCUDA/noinline.cu:1
+// optimization is needed, otherwise by default all functions have noinline.
+
----------------
I've asked @erichkeane to weigh in on whether there's a better approach here than specifying an optimization level.


================
Comment at: clang/test/SemaCUDA/noinline.cu:8
+__attribute__((noinline)) void fun2() { }
+__attribute__((__noinline__)) void fun3() { }
----------------
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() {}
```


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

https://reviews.llvm.org/D124866