[PATCH] D88345: [CUDA] Allow local `static const {__constant__, __device__}` variables.

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D88345#2298879 <https://reviews.llvm.org/D88345#2298879>, @jlebar wrote:

> OK, now I'm starting to I understand this change..
>
> Before, in function scope, we allow static const/non-const `__shared__`, and allow static const so long as it's not `__device__` or `__constant__`.
>
> - `static` -> error?  (I understood us saying above that it is, but now that I read the code, isn't it saying it's an error?)
> - `static const` -> allowed
> - `static __device__` -> error
> - `static const __device__` -> error
> - `static __constant__` -> error
> - `static const __constant__` -> error
>
> After, in function scope, the rule is, allow static const/non-const `__shared__` or anything that's `static const`.
>
> - `static` -> error, must be const
> - `static const` -> allowed
> - `static __device__` -> error, must be const
> - `static const __device__` -> allowed
> - `static __constant__` -> error, must be const
> - `static const __constant__` -> allowed
>
> I guess my question when I write out this table is, why shouldn't it be like this?
>
> - `static` -> allowed
> - `static const` -> allowed
> - `static __device__` -> allowed
> - `const static __device__` -> allowed
> - `static __constant__` -> error, must be const
> - `const static __constant__` -> allowed

It should. I did mention in a previous comment that `> Looks like the const-ness check should not be there, either`.
I need to revise the patch.

> This makes some sense to me because we're saying, "`__constant__` must be const", otherwise, anything goes.

Except that NVCC allows non-const `__constant__`, too. Generally speaking, C++ does not care about the attributes. While technically `__constant__` is not changeable from the device code, not specifying `const` is a missed optimization/diagnostic opportunity, but not an error per se. It does not affect how the variable is emitted, but rather what user can do with it and that's beyond the scope of this patch. I don't think it warrants a hard error. A warning, perhaps, that non-const `__constant__` is probably an error?

> Or here's another way of thinking about it.  You're saying that `static` and `static __device__` in function scope are the same as a `__device__` variable in block scope.  And a `__device__` variable in block scope doesn't have to be const (right?).  So why the extra restriction on function-scope static?

Something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88345