[SYCL] Refactor vec::convert implementation
#11770
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduced ability to perform vector conversions using a single instruction instead of doing so per-element.
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
added a commit
to AlexeySachkov/llvm
that referenced
this pull request
Nov 8, 2023
This is a bugfix for regressions caused by intel#11770 The following cases are fixed/updated: - Signed to unsigned (and vice versa) conversions are made to always go through per-element path, because native SPIR-V instruction behavies differently from what SYCL CTS expects. Strictly speaking, this could be a CTS issue, because `vec::convert` is underspecified (see KhronosGroup/SYCL-Docs#492), but until the spec is clarified we will stick with existing behavior; - Conversions to bool are made to always go through per-element path, because under the hood bool is represented as an integer and there is no guarantee that regular integer conversion will produce the right bit patterns for `true` and `false` which are not defined by any specification; Added an on-device test to check that `vec::convert` for integer types behaves correctly and consistently on both host and device. The test uncovered that we have a problem with `std::byte` and `vec::convert`, but it seems like we have never tested a case like this and it may not be even a regression. In any case it will be fixed in a separate PR.
This was referenced Nov 8, 2023
AlexeySachkov
added a commit
to AlexeySachkov/llvm
that referenced
this pull request
Nov 14, 2023
This reverts commit f634cc9.
AlexeySachkov
added a commit
that referenced
this pull request
Nov 20, 2023
This is a bugfix for regressions caused by #11770 The following cases are fixed/updated: - Signed to unsigned (and vice versa) conversions are made to always go through per-element path, because native SPIR-V instruction behavies differently from what SYCL CTS expects. Strictly speaking, this could be a CTS issue, because `vec::convert` is underspecified (see KhronosGroup/SYCL-Docs#492), but until the spec is clarified we will stick with existing behavior; - Conversions to bool are made to always go through per-element path, because under the hood bool is represented as an integer and there is no guarantee that regular integer conversion will produce the right bit patterns for `true` and `false` which are not defined by any specification; Added an on-device test to check that `vec::convert` for integer types behaves correctly and consistently on both host and device.
AlexeySachkov
deleted the
private/asachkov/native-vec-convert-implementation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduced ability to perform vector conversions using a single instruction instead of doing so per-element.