[SYCL] Disallow zero length arrays in SYCL kernels

[cperkinsintel]

zero length arrays in SYCL kernels are not allowed. Adding code to issue a deferred diagnostic error.

Signed-off-by: Chris Perkins [email protected]

[erichkeane]

I want to understand the delayed diagnostics bug you found while testing this before accepting this review.

[AGindinson]

@erichkeane, I understand "issue" was used as a verb.
@cperkinsintel, I'd suggest rewording the commit message to align with the common style:
[SYCL] Disallow zero length arrays in kernels

[erichkeane]

@erichkeane, I understand "issue" was used as a verb.

Not sure what you mean?

In this case, when staging this patch @cperkinsintel discovered a problem where his diagnostic wasn't firing in a certain common lambda construction. I'm simply suggesting we hold onto this patch until I understand that bug.

[AGindinson]

@erichkeane, I understand "issue" was used as a verb.

Not sure what you mean?

In this case, when staging this patch @cperkinsintel discovered a problem where his diagnostic wasn't firing in a certain common lambda construction. I'm simply suggesting we hold onto this patch until I understand that bug.

Sorry for the misunderstanding - I wasn't aware of any discussion taking place elsewhere, so I wrongly assumed your comment addressed the commit message's wording 🥇

[erichkeane]

@erichkeane, I understand "issue" was used as a verb.

Not sure what you mean?
In this case, when staging this patch @cperkinsintel discovered a problem where his diagnostic wasn't firing in a certain common lambda construction. I'm simply suggesting we hold onto this patch until I understand that bug.

Sorry for the misunderstanding - I wasn't aware of any discussion taking place elsewhere, so I wrongly assumed your comment addressed the commit message's wording 1st_place_medal

:D Makes sense. I'm only back home from Prague a few hours now (and am not working today) so I know I did a poor job clarifying my position on this PR before we figure out the problem.

[keryell]

What is the problem with 0-size array in SYCL?

[cperkinsintel]

@keryell ,

int BadArray[0]; A zero sized array declaration in a SYCL kernel, (or in a function called by a SYCL kernel) will be prevent linking. So trying to escalate the error into the compile phase.

zero sized arrays outside SYCL kernels are not rejected (and do not cause link failures)

[keryell]

@keryell ,

int BadArray[0]; A zero sized array declaration in a SYCL kernel, (or in a function called by a SYCL kernel) will be prevent linking. So trying to escalate the error into the compile phase.

zero sized arrays outside SYCL kernels are not rejected (and do not cause link failures)

I am curious about what is specific to SYCL for this case. Is this mentioned in the specification?
While zero sized array usage seems like a bad idea, allowing a declaration or a definition without using it seems OK, specially as a side effect of metaprogramming.
The dead-code elimination passes should remove the symbol anyway and should not even be visible by the linker. But probably you have a use case I do not see.

[Naghasan]

@keryell ,
int BadArray[0]; A zero sized array declaration in a SYCL kernel, (or in a function called by a SYCL kernel) will be prevent linking. So trying to escalate the error into the compile phase.
zero sized arrays outside SYCL kernels are not rejected (and do not cause link failures)

I am curious about what is specific to SYCL for this case. Is this mentioned in the specification?

It is in the C++ specs, it is illegal http://eel.is/c++draft/dcl.array#1
clang allows it because it is a GNU extension: https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

While zero sized array usage seems like a bad idea, allowing a declaration or a definition without using it seems OK, specially as a side effect of metaprogramming.
The dead-code elimination passes should remove the symbol anyway and should not even be visible by the linker. But probably you have a use case I do not see.

You make the validity of the code depends on optimizations, what if someone wants to run its code at O0 ?

[bader]

I think we can use err_typecheck_zero_array_size diagnostics.

[cperkinsintel]

If we do, that will output the message "zero-length arrays are not permitted in C++" when a zero length array is encountered from a SYCL kernel ( * ). Is that ok?

I think the "zero-length arrays are not permitted in SYCL kernels" is more accurate. But the decision is yours, @bader

( * ) - Zero length arrays elsewhere will still be accepted ( and I don't think we should change that behavior).

[bader]

If you want customize the message, you can make this diagnostics parameterized and emit different wording for SYCL.
I'm fine with the original message.

[keryell]

Thank you for the clarification.
So I reiterate my question: what is specific to SYCL here? Where is it specified in the specification? Either we are using some extensions or are strictly ISO C++ conformant, it seems orthogonal to SYCL...
Are we talking about some real user code here causing trouble with SYCL?

[Naghasan]

Thank you for the clarification.
So I reiterate my question: what is specific to SYCL here? Where is it specified in the specification? Either we are using some extensions or are strictly ISO C++ conformant, it seems orthogonal to SYCL...
Are we talking about some real user code here causing trouble with SYCL?

Your comment makes a bit more sense now, sorry...

I haven't dug much, but I think 0 length array can't be represented in SPIR-V https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#OpTypeArray

[cperkinsintel]

Was asked to move PR from a private fork and to change diagnostic message. Have done both in a new PR here: #1153

Am closing this one.