Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL][ESIMD] Add test for esimd default constructor #565

Merged
merged 26 commits into from
Dec 8, 2021

Conversation

vasilytric
Copy link

@vasilytric vasilytric commented Nov 11, 2021

This PR provides test for simd default constructor.
The test verifies that simd's elements value is equal to default value of element type
Also this PR provides common files:

  • function for printing verbose messaged into console
  • pre-defined sets of types for testing
  • function, that let iterating over pre-defined set of datatypes

@yuriykoch
Copy link

Please ensure you follow the LLVM Developer Policy as required by CONTRIBUTING.md.
More specifically, aren't the "add common code" and the "add test" commit messages too broad?

Also do we really need a separate "apply clang-format" commit? I mean, it looks like you were preparing the clean commit history before publishing your code anyway..

@vasilytric
Copy link
Author

Please ensure you follow the LLVM Developer Policy as required by CONTRIBUTING.md.
More specifically, aren't the "add common code" and the "add test" commit messages too broad?

Commits messages was changed after force pushing

Also do we really need a separate "apply clang-format" commit? I mean, it looks like you were preparing the clean commit history before publishing your code anyway..

This commit was created after CI running and clang-format issues was found, but now this commit was deleted due to force pushing

Copy link

@yuriykoch yuriykoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks great in general

@yuriykoch
Copy link

This commit was created after CI running and clang-format issues was found

My bad. I've missed it, sorry for that

@vasilytric
Copy link
Author

@kbobrovs @v-klochkov could you review this test for esimd default constructor?

@kbobrovs kbobrovs requested a review from v-klochkov November 23, 2021 05:14
@yuriykoch
Copy link

I would still strongly recommend to use api/functional/common.hpp instead of api/functional/ctors/common.hpp
as discussed earlier.
I can see no reason why we don't need such header for other functional tests and why we should move it later.

@vasilytric
Copy link
Author

I would still strongly recommend to use api/functional/common.hpp instead of api/functional/ctors/common.hpp
as discussed earlier.
I can see no reason why we don't need such header for other functional tests and why we should move it later.

OK, I moved this file into SYCL/ESIMD/api/functional in this commit: 18c93a1

Fix text of the file description, update includes: fix path in includes
due to some path have extra "../" and it is not correspond to current
path, on Windows it was OK, but on Linux it could be compilation error
@v-klochkov
Copy link

Please fix the test, see the CI fail at the newly added test.

- Were reworked comments that can rot
- Were changed some other comments
simd can't be constructed with sycl::half data type, i've done it for
successfully running GitHub CI
List of required platforms should be separated by commas, it is required
by the document "LLVM Testing Infrastructure Guide"
@vasilytric
Copy link
Author

Please fix the test, see the CI fail at the newly added test.

The CI on Linux and Windows are passed, CI on cuda failed (expected to fail due to failures in previous runnings) due to another tests failures but test for simd constructor should be skipped on cuda
For issue with sycl::half was created bug on intel/llvm: intel/llvm/issues/5077

Also at the beginning of the tests was added comment about situation with sycl::half, it can be found here

@v-klochkov do you have any other comments to this PR? can we merge it?

@vasilytric
Copy link
Author

@v-klochkov CI on windows failed due to built-ins/vector_relational.cpp test, the test simd_ctor_default.cpp is not failed, so this failure is not related to reviewed test

@vasilytric
Copy link
Author

@v-klochkov can we merge this PR?

@vladimirlaz vladimirlaz merged commit 17894ea into intel:intel Dec 8, 2021
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants