[SYCL] Fix integer vec conversions #11821
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
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.
|
@intel/sycl-language-enabling-triage: FYI |
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
reviewed
Nov 9, 2023
AlexeySachkov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
0x12CC
reviewed
Nov 10, 2023
sycl/include/sycl/types.hpp
Outdated
| // Whole vector conversion can only be done, if: | ||
| constexpr bool canUseNativeVectorConvert = | ||
| #ifdef __NVPTX__ | ||
| // - we are on CUDA, see intel/llvm#11840 |
There was a problem hiding this comment.
Should this comment say we are not on CUDA, see intel/llvm#11840?
There was a problem hiding this comment.
I understand the comment as "We are running on CUDA, see #11840 to understand why this is directly false.". Anyway, if it can lead to misunderstanding, we could clarify the comment to make sure it's understood by everyone in the same way.
There was a problem hiding this comment.
It should indeed contain not, because this line is a continuation of a previous statement, i.e. it should be read as:
Whole vector conversion can only be done, if:
- we are not on CUDA
- other conditions required for whole vector conversions
Added "not" in as part of merge commit (311ba07)
…integer-vec-conversions This commit also fixes conversions to/from `std::byte`
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
commented
Nov 16, 2023
|
|
||
| if constexpr (std::is_same_v<convertT, bool>) { |
There was a problem hiding this comment.
@frasercrmck, FYI, this PR re-implements your fix
…integer-vec-conversions
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
deleted the
private/asachkov/fix-integer-vec-conversions
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.
This is a bugfix for regressions caused by #11770
The following cases are fixed/updated:
vec::convertis underspecified (seevec::convertis underspecified KhronosGroup/SYCL-Docs#492), but until the spec is clarified we will stick with existing behavior;trueandfalsewhich are not defined by any specification;Added an on-device test to check that
vec::convertfor integer types behaves correctly and consistently on both host and device. The test uncovered that we have a problem withstd::byteandvec::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.