Add support for SimSYCL as a SYCL implementation #871
Conversation
cmake/AddSYCLExecutable.cmake
Outdated
| if (NOT ${SYCL_IMPLEMENTATION} IN_LIST KNOWN_SYCL_IMPLEMENTATIONS) | ||
| message(FATAL_ERROR | ||
| "The SYCL CTS requires specifying a SYCL implementation with " | ||
| "-DSYCL_IMPLEMENTATION=[Intel_SYCL,DPCPP;hipSYCL]") | ||
| "-DSYCL_IMPLEMENTATION=[Intel_SYCL,DPCPP;hipSYCL;SimSYCL]") |
There was a problem hiding this comment.
I wonder if we should just put KNOWN_SYCL_IMPLEMENTATIONS directly into the message instead of modifying it every time we change list of known implementations?
There was a problem hiding this comment.
Perhaps CTS_GRADE_SYCL_IMPLEMENTATION or something better, because we know quite many other implementations. :-)
tests/event/event.cpp
Outdated
| #if SYCL_CTS_COMPILING_WITH_SIMSYCL | ||
| SKIP("SimSYCL does not implement asynchronous execution."); | ||
| #endif |
There was a problem hiding this comment.
I think that from the conformance point of view the better to use DISABLED_FOR_TEST_CASE macro so the test appears as failed.
There was a problem hiding this comment.
I'm not sure this is even non-conformant. IIRC an implementation only needs to guarantee forward progress on an event when wait is called on it, which does not happen in any of these test cases.
There was a problem hiding this comment.
I don't think this test requires a guarantee of forward progress, does it? It seems like it is just calling event::get_wait_list and making sure the list contains the direct dependencies of a pending command. There is no expectation that the command has completed when get_wait_list is called.
| if(SYCL_IMPLEMENTATION STREQUAL SimSYCL) | ||
| message(WARNING "SimSYCL does not provide true concurrency between host and device, disabling USM atomic tests") | ||
| list(FILTER test_cases_list EXCLUDE REGEX usm_atomic_access_.*\\.cpp$) | ||
| endif() |
There was a problem hiding this comment.
Piggybacking off of @AlexeySachkov's comment, these should preferably also be disabled using DISABLE_FOR_TEST_CASE(SimSYCL, ...!
There was a problem hiding this comment.
Same as above - this needs a forward progress guarantee in SYCL to pass.
|
Please also document SimSYCL in |
|
Ideally we should also include SimSYCL in our CI setup. I think it should be relatively straightforward to add, analogously to hipSYCL/AdaptiveCpp. |
|
How do we move on this? |
fknorr
force-pushed
the
build-with-simsycl
branch
from
ae3274e to
7d5f825
Compare
fknorr
force-pushed
the
build-with-simsycl
branch
from
7d5f825 to
a330782
Compare
fknorr
force-pushed
the
build-with-simsycl
branch
from
3c601ad to
28402a6
Compare
fknorr
force-pushed
the
build-with-simsycl
branch
from
28402a6 to
47b7b26
Compare
|
I've added a CI step for a SimSYCL build, but creating the docker image needs some secrets. Not sure how to proceed on this. |
I've launched a manual run to create the image here. It went through, but it looks like there's some issue while building the CTS itself! |
|
Thanks, it's building now! |
atomicity_with_host_code requires a memory_scope argument (always `system`). atomic_ref<float> has no operator++ and must use fetch_add() instead.
fknorr
force-pushed
the
build-with-simsycl
branch
from
6580cf8 to
66159bf
Compare
This adds SimSYCL as a supported SYCL implementation, complete with CMake integration and an exclude-list for the "fast" conformance build.
This should be merged after #870 #872 and #873, because the CTS it will not actually compile for many of the non-excluded cases unless those bugs are addressed.