[SYCL] no more ast visitor usage for variable type checks. #1513
Conversation
Signed-off-by: Chris Perkins <chris.perkins@intel.com>
…etired as well Signed-off-by: Chris Perkins <chris.perkins@intel.com>
Signed-off-by: Chris Perkins <chris.perkins@intel.com>
| @@ -36,7 +36,7 @@ void no_restriction(int p) { | |||
| int index[p + 2]; | |||
| } | |||
| void restriction(int p) { | |||
| int index[p + 2]; // expected-error {{variable length arrays are not supported for the current target}} | |||
| int index[p + 2]; // expected-error 2{{variable length arrays are not supported for the current target}} | |||
There was a problem hiding this comment.
What are the two errors here? How'd we get the extra one? I'd like to understand how this happens.
There was a problem hiding this comment.
I have a theory .
Everything else in the isa_B function context has double the number of expectation values than you would expect. I think this is because the calling hierarchy looks like this:
main -> kernel -> usage -> kernel -> isa_B -> restriction
Note that there is a kernel inside the usage function, which is called from a different kernel.
The change in this PR moves the diagnostics for VLA to be a Deferred Diagnostic, like most of the others. The Deferred Diagnostics walk the final produced kernel looking to see if there were any warnings deferred earlier for that thing, and then they emit.
So my theory is the the old system, using the AST Visitor, only visited it once (and then issued an immediate diagnostic). But with this change ,the deferred diagnostic system essentially visits it twice, because of the nested kernels.
I did a quick test and move that call to restriction() up a level to usage() , and now it fires just once, not twice.
There was a problem hiding this comment.
I can make that change to the sycl-restrict, to make it clearer. I do not know why there are nested kernels for these tests. Let me know if you would like a change.
There was a problem hiding this comment.
Right, I just want to make sure its still an appropriate amount. sycl-restrict is criminally oversized unfortunately, so understanding this stuff is annoying. If there is a simplification that you can makes it clearer, I would be appreciative.
There was a problem hiding this comment.
I there any reason for having nested kernels in these tests? Or is it just an artifact of this files history?
I'd be happy to get rid of the kernel nesting which would tidy things up a bit. But I don't want to undo anything important.
Otherwise, I can simply move the testing of the VLA . My own inclination is to get rid of the nested kernels entirely.
There was a problem hiding this comment.
Sorry to be dense. Like this?
| int index[p + 2]; // expected-error 2{{variable length arrays are not supported for the current target}} | |
| // expected-note@71 2{{called by 'isa_B'}} | |
| int index[p + 2]; // expected-error 2{{variable length arrays are not supported for the current target}} |
Is there another way of using the "@" syntax so it's not absolute line numbers? I have used the relative "@+1" to put these immediately before a line, but in the case we are discussing, the caller we are mentioning is far away. This approach seems like it'll just break the minute someone edits the file.
Is this -verify expectation system documented anywhere? I've searched, but the llvm-lit docs don't talk about this part. Are there other ways of using the "@" in the expectation?
There was a problem hiding this comment.
There was a problem hiding this comment.
thanks. A veritable rosetta stone, that. Do you have the decipher key for Linear A as well?
There was a problem hiding this comment.
Ok, I have pushed added markers and moved the notes into reasonable places for the VLA test, which is the one test this PR touches.
I would like to just go ahead and finish that and move all the stray note expectations in sycl-restrict.cpp and put them close to their caller. Is that ok to do in this PR? Or should I open a different one for that?
…for the vla error Signed-off-by: Chris Perkins <chris.perkins@intel.com>
Signed-off-by: Chris Perkins <chris.perkins@intel.com>
…c_abi_checks * origin/sycl: [SYCL][Driver] Enforce unique filenames when -save-temps is used (intel#1545) [SYCL] [xmethods] Allow replacing xmethod script (intel#1532) [SYCL] Add tests for inline asm feature (intel#1444) [SYCL][Doc] Add device_specific_kernel_queries extension. (intel#1540) [SYCL][USM] Remove unused header and unnecessary includes (intel#1537) Fix check-llvm dependencies (intel#1547) [SYCL] Add __SYCL_EXPORT to declaration of contextSetExtendedDeleter (intel#1531) [SYCL][Doc] Add static local memory query extension. (intel#1539) [SYCL][Doc] Add sycl_bitcast extension (intel#1541) [SYCL][NFC] Temporarily disable sporadically failing test (intel#1533) [SYCL][NFC] Adjust codeowners for sycl directory (intel#1529) [SYCL] Fix processing of spec consts referenced twice (intel#1524) [SYCL] Use correct macro name in export.hpp (intel#1527) [Driver][NFC] Fix -help information for -Xs options (intel#1530) [SYCL][Doc] Add Graph Scheduler design documentation (intel#1457) [SYCL] Add diagnostics for long double in device code (intel#1512) [SYCL] Add a mutex to state-modifying program functions (intel#1204) [SYCL][Test] Add Devicelib tests (intel#1256) [SYCL] Refactor semantic checks for variable types (intel#1513)
The various type checks have been steadily moving out of
CheckSYCLTypewhich is called by most of the AST Visitor methods. Here we finally move the last lingering type check (for VLAs) into theCheckSYCLVarTypefunction and deleteCheckSYCLTypeand most of its AST Visitor method callers.Some few of the AST Visitor methods are still used for other checks.