[SYCL] Indicate lack of direct OpenCL interop when not using OpenCL BE. #1242
Conversation
Signed-off-by: Sergey V Maslov <[email protected]> Co-Authored-By: Rehana Begam <[email protected]> Alexey Bader <[email protected]>
|
please review this @bjoernknafla |
| @@ -1,3 +1,5 @@ | |||
| // REQUIRES: opencl | |||
There was a problem hiding this comment.
I assume it means that this test is supposed to be skipped if there is no OpenCL support in the system.
If so, the code which detects presence of OpenCL looks strange:
# PI API either supports OpenCL or CUDA.
opencl = False
if not cuda:
opencl = True
config.available_features.add('opencl')
There was a problem hiding this comment.
The current test idea is to either run testing for OpenCL xor CUDA.
(My understanding is though that some tests do not go through the SYCL default selector and therefore won't react to the env var - this adds some nondeterminism to which backend they choose).
The selection happens as follows:
- If the
check-sycltarget is called, then we want to test OpenCL. - If the
check-sycl-cudatarget is called, then we want to test CUDA. - By hand this would either be:
**./bin/llvm-lit --param SYCL_BE=PI_OPENCL ./tools/sycl/testor
**./bin/llvm-lit --param SYCL_BE=PI_CUDA ./tools/sycl/test
The Python script for the LIT SYCL tests uses the SYCL_BE environment variable to pass a backend parameter to the get_device_count_by_type tool which only reports the available devices belonging to the parameter. It won't report CUDA devices when looking for OpenCL and vice versa.
Therefore, we either set the cuda xor the opencl feature but not both together.
However, it is always possible to explicitly query for all devices of all backends by not using the default device selector.
There was a problem hiding this comment.
I don't have neither OpenCL nor CUDA on my machine. What will happen to my check-sycl?
There was a problem hiding this comment.
Just to clarify, I expect my tests to run on the host device only, but I'm not sure that tests which explicitly requested OpenCL will be disabled.
There was a problem hiding this comment.
Good question. I am not sure how to solve this cleanly right now, e.g., we would want to say that no backend is under test (to express that we test HOST)?
What will happen at the moment is:
- OpenCL testing is activated.
- No devices are found by the
get-device-count-by-typetool which limits the RUN lines - so many test might skip while some fail. - Tests themselves still find all available devices at runtime.
- Some tests explicitly check that they are not on host before calling into OpenCL.
If I run ninja check-sycl on the tip of sycl on a machine without OpenCL but with CUDA I get the following:
Testing Time: 158.27s
********************
Failing Tests (5):
SYCL :: kernel_from_file/hw.cpp <- unexpected interaction with CUDA
SYCL :: scheduler/DataMovement.cpp <- unexpected interaction with CUDA
SYCL :: scheduler/MultipleDevices.cpp <- unexpected interaction with CUDA
SYCL :: separate-compile/test.cpp <- unexpected interaction with CUDA
Expected Passes : 201
Unsupported Tests : 17
Unexpected Failures: 5On a machine without OpenCL and when not building the CUDA backend no failures happen.
Something to look into though not in this PR I recon.
There was a problem hiding this comment.
NOTE: this file already has // REQUIRES: opencl at line 7.
There was a problem hiding this comment.
@bjoernknafla, I'm okay to resolve this separately. I need to go deeper into LIT config file to understand what is the best way to handle it.
There was a problem hiding this comment.
My bad, I missed the // REQUIRES: opencl at line 7. Let me check if I've missed any more.
| @@ -1,3 +1,5 @@ | |||
| // REQUIRES: opencl | |||
|
|
|||
| // RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out -L %opencl_libs_dir -lOpenCL | |||
| // RUN: env SYCL_DEVICE_TYPE=HOST %t.out | |||
| // RUN: %CPU_RUN_PLACEHOLDER %t.out | |||
There was a problem hiding this comment.
Should we remove lines 9 and 10?
// TODO: Image support in CUDA backend
// XFAIL: cudaThere was a problem hiding this comment.
It would not hurt though might be better as an extra PR to not block this one that re-enables LIT testing with CUDA?
There was a problem hiding this comment.
I'm okay to resolve this comment in a separate pull request.
@bjoernknafla, will do this?
We need to clean-up following files from this PR:
basic_tests/buffer/buffer.cpp
basic_tests/sampler/sampler.cpp
fpga_tests/fpga_queue.cpp
ordered_queue/ordered_buffs.cpp
ordered_queue/ordered_dmemll.cpp
sub_group/common_ocl.cpp
I think all these files should have something like:
// REQUIRES: opencl
// UNSUPPORTED: cudaThere was a problem hiding this comment.
If I remember correctly @bjoernknafla mentioned that XFAIL: cuda was there to indicate that the test fails for CUDA and needs to be fixed. Have all the TODOs been resolved for ^ tests?
There was a problem hiding this comment.
These are requiring OpenCL, so I assume that none of them can be "fixed on CUDA". Am I missing something?
There was a problem hiding this comment.
sampler.cpp should have a UNSUPPORTED: cuda on it.
If the others already have REQUIRES: opencl nothing else should be required. However, I will look over this in an upcoming PR, so we could get LIT fixes of this PR in.
|
@rbegam, please, revert |
Signed-off-by: Rehana Begam <[email protected]>
rbegam
force-pushed
the
private/rbegam/sycl-interop
branch
from
a1d7de1 to
051f200
Compare
|
@bader done. please review |
|
hi @bader could you please review? thanks. |
Signed-off-by: Sergey V Maslov [email protected]
Co-Authored-By: Rehana Begam [email protected]
Alexey Bader [email protected]