[SYCL] Reject zero length arrays

[cperkinsintel]

This moves this work so it originates from a private fork. Also incorporated in the feedback.

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

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

[bader]

@cperkinsintel, please, fix LIT test failures.
@erichkeane, @Fznamznon, please, take a look at unresolved comments.

[Fznamznon]

Why this array A is needed at all?
The problem with diagnostics appears only if single_task function is called from a template function?

[cperkinsintel]

Correct. The diagnostics do not emit if the lambda that contains the violations appears in a templated function. So I'm just using the array to specialize the template.

[Fznamznon]

What exactly do you mean here?
BTW, we diagnose only usages of virtual functions, as SYCL spec requires. Using of classes with virtual functions is not prohibited.

[cperkinsintel]

I am not sure if this particular violation ( a virtual function in SYCL context ) results in a deferred diagnostic, as opposed to an immediate diagnostic. @erichkeane added it to one of the files when he was working on reducing the problem. So I kept it in the test.

[Fznamznon]

AFAIK we diagnose virtual functions usage through SemaSYCL callgraph traversing. See

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 233 in f87291d

<< Sema::KernelCallVirtualFunction;

. But, I suppose, we will move this to Sema and deferred diagnostics.

But we already diagnose variadic function calls through deferred diagnostic, I remember patch from Erich doing this #998 .
So If here you are trying to check only deferred diagnostics, I suggest switch from virtual functions to variadic functions.

[cperkinsintel]

Ok, I switched to variadic.

[cperkinsintel]

@Fznamznon
Can you help with with this test? I have one last "note" I need to suppress.

When running llvm-lit -v deferred-diagnostics-emit.cpp I get this:

error: 'note' diagnostics seen but not expected: 
  File /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/Inputs/sycl.hpp Line 182: called by 'kernel_single_task<AName, (lambda at /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp:61:34)>'
  File /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/Inputs/sycl.hpp Line 182: called by 'kernel_single_task<AName, (lambda at /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp:61:34)>'

So it looks like I should add this expectation:

// expected-note@+1 2{{called by 'kernel_single_task<AName, (lambda}}

But no matter where I put that expectation it never works. First it tells me it expected the note but didn't see it, then it says it saw the note without expecting it. Awesome.

error: 'note' diagnostics expected but not seen: 
  File /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp Line 61 (directive at /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp:60): called by 'kernel_single_task<AName, (lambda
  File /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp Line 61 (directive at /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp:60): called by 'kernel_single_task<AName, (lambda
error: 'note' diagnostics seen but not expected: 
  File /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/Inputs/sycl.hpp Line 182: called by 'kernel_single_task<AName, (lambda at /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp:61:34)>'
  File /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/Inputs/sycl.hpp Line 182: called by 'kernel_single_task<AName, (lambda at /iusers/cperkins/sycl_workspace/llvm/clang/test/SemaSYCL/deferred-diagnostics-emit.cpp:61:34)>'

Is the expectation correct? I haven't been able to find docs for them, so I'm just cribbing from other places.

I tried putting the expectation at line 58, line 60, in the argument list of cgh.single_task so at to precede the lambda, line 62, line 76, etc. But nothing works.

[cperkinsintel]

Is there a compiler flag to suppress the notes?

[Fznamznon]

First, I don't think that we should suppress notes in tests.
Second, I see what happens. You are trying to add note expectation to wrong place.
See logs, the emitted note points to function named kernel_single_task and it's defined in file test/SemaSYCL/Inputs/sycl.hpp on line 182.
By the construction like // expected-note@+1 2{{called by 'kernel_single_task<AName, (lambda}} in test/SemaSYCL/deferred-diagnostics-emit.cpp file on line 60 means that you expect 2 same notes with this text in this file on line 61.
The trick here that you use single_task function from our fake sycl runtime, and kernel_single_task call happens inside it. I'm not sure that I know how to expect notes from other file (maybe @erichkeane knows). As a fix you can declare your own kernel_single_task function in your test file and note expectation will point to correct file.

[cperkinsintel]

Ok - I re-introduced the bespoke SYCL namespace code into the test and put the expectation into it. Now the llvm-lit passes and the test correctly stresses the namespace aspect we require.

[Fznamznon]

I do not see the purpose of defining these classes, it is unnecessary and makes test fat. Can we just use one function with sycl_kernel attribute? Only this is needed to show the compiler where the device code starts from.

[cperkinsintel]

@Fznamznon
We cannot just throw the sycl_kernel attribute on a function like we do in some of the other tests - what we are looking for, specifically, is that the deferred diagnostics are working with namespaces.
Remember, there is that overhaul of the deferred diagnostic system which was merged and then backed out - that overhaul passed all existing tests but yet caused all of our SYCL deferred diagnostics to suddenly not work. The purpose of this deferred-diagnostics-emit.cpp is to provide defensive testing that will fail if that overhaul makes the same mistake and is later re-introduced. This code you are identifying came from the case where we isolated the problem when used against that overhaul. It has been confirmed against that. Before I can change it, I'll have to reconfirm.

This PR has been open for over a week now, and is really quite small. This particular test file is defensive. I would prefer that we merge it because we all have other things demanding our attention. I can look into trimming this side test file, if you insist, but my preference is to merge this PR rather than having it languish another week. Let me know.

[Fznamznon]

I understand that you want to capture some problems and we won't forget about them, I appreciate it. But all that I'm asking is reduce the test case to have minimal reproducer for our problems. You want capture that we have problems with namespaces - use namespaces, instead of adding not necessary stuff. All these classes will appear in AST dumps for module and will make debugging harder.
BTW. We have tons of ugly/bad/unnecessary changes in SYCL just because someone was running out of time. So the fact that it's "long conversation"/"we don't have time", "someone very needed to merge it yesterday" etc, won't force me to give approve for the patch if this patch doesn't fix crash which happens in the compiler on every second code sample.

[cperkinsintel]

done.

[cperkinsintel]

ping @Fznamznon - I think I have addressed everything you brought up when reviewing this PR. It should be ready to be approved and merged. Let me know if not.

[romanovvlad]

@bader @erichkeane Could you please finish review?

[erichkeane]

I was OK with it a while ago, Up to @Fznamznon as far as I'm concerned.

[erichkeane]

Spent a while looking at this based on @Fznamznon 's comments, and I think this covers it.

[cperkinsintel]

@erichkeane - thanks! I think you'll have to merge it as well, as I don't have authorization for that.

[erichkeane]

@bader is the only one I know of that can merge anything, I also lack permissions to do so.