Compatibility: Only use cl_* types if SYCL_CTS_ENABLE_OPENCL_INTEROP_TESTS is set
#873
Conversation
fknorr
changed the title
cl_* types if SYCL_CTS_ENABLE_OPENCL_INTEROP_TESTS is set
| @@ -37,8 +37,10 @@ class TEST_NAME : public util::test_base { | |||
| // Test using queue constructed already | |||
| for_type_and_vectors<check_type, sycl::half>(queue, log, | |||
| "sycl::half"); | |||
| #if SYCL_CTS_ENABLE_OPENCL_INTEROP_TESTS | |||
| for_type_and_vectors<check_type, sycl::cl_half>(queue, log, | |||
| "sycl::cl_half"); | |||
There was a problem hiding this comment.
The tests seem to be outdated.
Spec says:
For the purpose of interoperability and portability, SYCL defines a set of aliases to C++ types within the sycl::opencl namespace using the cl_ prefix.
| "sycl::cl_half"); | |
| "sycl::opencl::cl_half"); |
Right?
There was a problem hiding this comment.
I've updated most uses of cl_*, however there's the cl_float4 etc vector types which also seem to be deprecated, and DPC++ doesn't alias them into opencl::. I've left those alone.
There was a problem hiding this comment.
there's the
cl_float4etc vector types which also seem to be deprecated, and DPC++ doesn't alias them intoopencl::.
@steffenlarsen, could you please check the DPC++, please? I found intel/llvm@e98d732 by @AlexeySachkov, which adds aliases for scalar types, but I don't see definitions for vector aliases in https://github.com/intel/llvm/blob/e89da3251187ec101f995a8dbf6e832043baebdc/sycl/include/sycl/aliases.hpp.
We need to create trackers to fix both CTS tests (i.e. sycl::cl_float4 -> sycl::opencl::cl_float4) and DPC++ implementation (i.e. implement sycl::opencl::cl_float4).
There was a problem hiding this comment.
I found the spec defines aliases only for scalar OpenCL types https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#_data_types_2.
The OpenCL C language standard Section 6.11 defines its own built-in scalar data types, and these have additional requirements in terms of size and signedness on top of what is guaranteed by ISO C++. For the purpose of interoperability and portability, SYCL defines a set of aliases to C++ types within the sycl::opencl namespace using the cl_ prefix. These aliases are described in Table 182.
I'm not sure what is the reason to skip vector types, but at the moment, we can open an issue for SYCL-CTS to replace OpenCL vector type aliases with sycl::vec type (i.e. sycl::cl_float4 -> sycl::vec<sycl::opencl::cl_float, 4>).
I recalled that @AlexeySachkov already filed an issue about missing vector aliases - KhronosGroup/SYCL-Docs#335, but it's still unresolved.
There was a problem hiding this comment.
I agree with you analysis, but it sounds like DPC++ is defining the right aliases until KhronosGroup/SYCL-Docs#335 is resolved. Please correct me if I'm wrong.
|
|
||
| #ifdef INT8_MAX | ||
| if (!std::is_same<sycl::cl_char, std::int8_t>::value) | ||
| for_type_and_vectors<check_type, std::int8_t>(log, "std::int8_t"); | ||
| for_type_and_vectors<check_type, std::int8_t>(log, "std::int8_t"); |
There was a problem hiding this comment.
This introduces duplication into the test. But you can move the if condition under #if SYCL_CTS_ENABLE_OPENCL_INTEROP_TESTS.
There was a problem hiding this comment.
Done, although this makes things a lot more verbose.
|
|
||
| #ifdef INT8_MAX | ||
| if (!std::is_same<sycl::cl_char, std::int8_t>::value) |
There was a problem hiding this comment.
I wonder how these tests could have been false at the first place.
There was a problem hiding this comment.
I don't think they can, integer widths are part of the spec.
|
@fknorr Some merge from upstream? |
fknorr
force-pushed
the
compat-cl-interop
branch
from
ce98390
to
de83de0
Compare
fknorr
force-pushed
the
compat-cl-interop
branch
from
de83de0
to
f0f71db
Compare
fknorr
force-pushed
the
compat-cl-interop
branch
from
e5b70f4
to
b7bcf5f
Compare
|
Should be good to go now. |
This improves compatibility with implementations that do not expose OpenCL interop (notably SimSYCL, and AFAIK also AdaptiveCpp / hipSYCL).
This change is probably incomplete, but enough to get some tests to build with SimSYCL that otherwise wouldn't.
group_async_work_group_copy.cpphad!is_samechecks between cl types and stdint types - please correct me if it's wrong to remove these checks, it appears to me as though performing the same check twice is not an issue - but maybe there's symbol clashes.