[SYCL][ESIMD] Add test for simd constructor from vector #687
Conversation
- Was updated comments - In logical statements "=" was replaced with "&=" to not averwrite value in test fail and the pass cases - Used simd.data() to retreive vector_type and then provide it to the simd constructor
- Update comments - Rename "input_data" to "ref_data" into kernels
|
@v-klochkov Vyacheslav, could you review this PR? |
| // XRUN: %GPU_RUN_PLACEHOLDER %t.out | ||
| // RUN: false | ||
| // XFAIL: * | ||
| // TODO The simd can't be constructed with sycl::half data type. The issue was |
There was a problem hiding this comment.
@kbobrovs added support for half types in simd type just recently. Please try the newer compiler.
There was a problem hiding this comment.
BTW, it may be Ok for this test not to check if HW supports those extra fp types because it only constructs an object and does not have any math operations, but in general such test would require such check before running on device, i.e. sycl::aspect::fp16
There was a problem hiding this comment.
Should we skip a test if device does not support any underlying type?
Should we do something with reference data if, for example nan is not supported by current device, but it should be in the reference data?
I have the following concern:
- We should do a few runtime verifications during reference values generation
- We should do verifications for each optional features in each test
There was a problem hiding this comment.
Perhaps, it can be guarded by runtime macro, something like:
https://github.com/intel/llvm-test-suite/blob/intel/SYCL/Reduction/reduction_nd_ext_half.cpp#L20
https://github.com/intel/llvm-test-suite/blob/intel/SYCL/Reduction/reduction_nd_ext_double.cpp#L20
https://github.com/intel/llvm-test-suite/blob/intel/SYCL/Reduction/reduction_nd_ext_type.hpp#L22
I am not sure what to do with 'nan' not supported on some device. The test should check common functionality.
If there is some device specific detail, it could be moved to a separate test that would be run only subset of devices (i.e. additional/special test would have more restrictions).
There was a problem hiding this comment.
I've added verification in this commit: 32fd77f
lets move additional discussions out of scope of this PR because of this affects all other tests on simd constructors.
I suggest create another one PR that provides logic for verification of all required optional device features.
There was a problem hiding this comment.
Ok, I agree, this is more general issue affecting many tests that are already merged, and it deserves a separate PR.
I mean the absence of fp16 support should not cause skip of the whole test (i.e. checks of regular types like int).
I approve now.
Please do not forget to handle types like half in one of nearest future PRs.
No description provided.