Add cases for *_vector_width_long_long#1182
Conversation
bader
left a comment
bader
left a comment
There was a problem hiding this comment.
@KornevNikita, please, fix CI checks.
|
No compiler supports this change yet, therefore adding under macros. |
|
No check for adativeCPP? But I guess the CI pass, so not required :) |
@TApplencourt ACPP doesn't support the whole block: SYCL-CTS/tests/device/device_info.cpp Lines 84 to 85 in cea4ae6 |
|
Thanks!
|
| uint32_t>(dev); | ||
| check_get_info_param<sycl::info::device::preferred_vector_width_long, | ||
| uint32_t>(dev); | ||
| #if !SYCL_CTS_COMPILING_WITH_SIMSYCL && !SYCL_CTS_COMPILING_WITH_DPCPP |
There was a problem hiding this comment.
I don't think we should disable tests via SYCL_CTS_COMPILING_WITH_DPCPP. DPC++ claims conformance to the CTS, so we can't do this honestly if we disable some of the tests. I do not think any other tests are currently disabled for DPC++ with this macro.
There was a problem hiding this comment.
DPC++ claims conformance to the CTS
My understanding DPC++ claims conformance to specific versions of the spec/CTS. It doesn't mean that any DPC++ version passes any CTS version.
The spec, implementation and CTS all are moving targets. Here we test new functionality, which DPC++ implementation doesn't support yet.
There was a problem hiding this comment.
If we don't want to use this macro, then DPCPP needs to support this, there is no another way, right?
Anyways, I can quickly implement some dummy solution so the test passes, WDYT?
There was a problem hiding this comment.
I was assuming that test would just fail for DPC++ until we implement this feature, and that failure would ensure that we implement the feature before we submit our next conformance.
If we instead disable the test with the macro, what process do we have to ensure that the test is re-enabled by the time we submit our next conformance? I'm concerned that if we add the macro now to disable the test, we will just forget about it, and then we'll end up claiming conformance in the future for a CTS with some tests disabled.
There was a problem hiding this comment.
Conformance logs which implementer sends the Khronos include SYCL-CTS commit hash and the diff applied locally on top of it (if any). Khronos working group is expected to raise concerns about disabled tests and not acknowledge tested implementation as conformant if any changes are considered to be unreasonable.
On DPC++ side, I expect @KornevNikita to handle it as usual (i.e. open a tracker, implement feature in DPC++, uplift DPC++ version in Khronos SYCL-CTS CI and remove guards).
There was a problem hiding this comment.
I think it is unreasonable to expect the WG to read through the entire test suite each time there is a conformance submission to see if tests have been disabled. Instead, the CTS should be structured to make this obvious from the test run submission. Can we create a cmake config option like -D DISABLE_FAILING_TESTS_DPCPP, and structure the CTS so that tests are only disabled when that option is specified? That way, it will be obvious from the submission whether tests have been disabled. It will also be obvious to vendors even before they attempt to submit a conformance run. My concern is not that a vendor will intentionally try to seek conformance for a failing implementation. Instead, I'm concerned that we will simply forget to re-enable tests after they have been disabled.
There was a problem hiding this comment.
I think it is unreasonable to expect the WG to read through the entire test suite each time there is a conformance submission to see if tests have been disabled.
I did this for the previous submission. It's quite easy because we use one standard macro to disable test parts per implementation. It takes less than a minute to check.
Instead, the CTS should be structured to make this obvious from the test run submission.
CTS provides an option to disable a test case. The submission log will state that test case is disabled.
Using this option for this change will mean that all device information query tests will be disabled.
|
WG: Agree to merge. |
4 participants
#1178