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

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.


================
Comment at: clang/lib/Basic/IdentifierTable.cpp:111
     KEYSYCL       = 0x1000000,
+    KEYCUDA       = 0x2000000,
     KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
----------------
yaxunl wrote:
> yaxunl wrote:
> > delcypher wrote:
> > > yaxunl wrote:
> > > > delcypher wrote:
> > > > > @yaxunl Is it intentional that you didn't update `KEYALL` here? That means `KEYALL` doesn't include the bit for `KEYCUDA`.
> > > > > 
> > > > > If that was your intention then this will break if someone adds a new key. E.g.
> > > > > 
> > > > > ```
> > > > > KEYCUDA = 0x2000000,
> > > > > KEYSOMENEWTHING = 0x4000000,
> > > > > // ...
> > > > > // KEYALL now includes `KEYCUDA`, whereas it didn't before.
> > > > > // KEYALL includes KEYSOMENEWTHING 
> > > > > KEYALL = (0x7ffffff & ~KEYNOMS18 &
> > > > >               ~KEYNOOPENCL) // KEYNOMS18 and KEYNOOPENCL are used to exclude.
> > > > > ...
> > > > > ```
> > > > > 
> > > > > 
> > > > > 1. Updating the `0x1ffffff` constant to `0x3ffffff` so that `KEYALL` includes `KEYCUDA`
> > > > > 2. If your intention **is** to not have `KEYCUDA`  set in `KEYALL` then amend `KEYALL` to be.
> > > > > 
> > > > > ```
> > > > > KEYALL = (0x7ffffff & ~KEYNOMS18 &
> > > > >               ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are used to exclude.
> > > > > // KEYCUDA is not included in KEYALL
> > > > > ```
> > > > My intention is not to include KEYCUDA in KEYALL.
> > > > 
> > > > Should I change KEYALL to
> > > > 
> > > > 
> > > > ```
> > > > KEYALL = (0x3ffffff & ~KEYNOMS18 &
> > > >               ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are used to exclude.
> > > > // KEYCUDA is not included in KEYALL
> > > > ```
> > > > 
> > > > instead of 
> > > > 
> > > > 
> > > > ```
> > > > KEYALL = (0x7ffffff & ~KEYNOMS18 &
> > > >               ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are used to exclude.
> > > > // KEYCUDA is not included in KEYALL
> > > > ```
> > > > 
> > > > since the current maximum mask is 0x3ffffff instead of 0x7ffffff
> > > Oops, you're right it would be `0x3ffffff`. I wonder though if we should clean this up so we don't need to manually update the bit mask every time... what if it was written like this?
> > > 
> > > ```lang=c++
> > >  enum {
> > >     KEYC99        = 0x1,
> > >     KEYCXX        = 0x2,
> > >     KEYCXX11      = 0x4,
> > >     ....
> > >     KEYSYCL       = 0x1000000,
> > >     KEYCUDA       = 0x2000000,
> > >     KEYMAX = KEYCUDA, // Must be set to the largest KEY enum value
> > >     KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
> > > 
> > >     // KEYNOMS18 and KEYNOOPENCL are used to exclude.
> > >     // KEYCUDA is not included in KEYALL because <FIXME add reason here>
> > >     KEYALL = (((KEYMAX & (KEYMAX-1)) & ~KEYNOMS18 & ~KEYNOOPENCL & ~KEYCUDA)
> > > };
> > > ```
> > On second thought, KEYALL does not need to exclude KEYCUDA.
> > 
> > However, it would be good to set KEYALL in a generic approach. I will open a separate review.
> opened https://reviews.llvm.org/D125396 to fix KEYALL
Oops that should say

```
KEYALL = (((KEYMAX | (KEYMAX-1)) & ~KEYNOMS18 & ~KEYNOOPENCL & ~KEYCUDA)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124866