Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix defects in vector check_* functions #756

Merged
merged 6 commits into from
Aug 1, 2023

Conversation

0x12CC
Copy link
Contributor

@0x12CC 0x12CC commented Jul 21, 2023

Change the return type of check_as_result to bool to match its definition.

Fix check_convert_as_all_dims and check_convert_as_all_types to combine check_* results using the &= operator rather than the += operator. This change is consistent with the definition of check_vector_convert_modes.

Fix check_vector_convert_result to use the rte or rtz conversion functions when the mode is rounding_mode::automatic.

Fix `check_as_result` to prevent reading from uninitialized memory for
cases where `(N * sizeof(vecType)) < (asN * sizeof(asType))`. Also,
change its return type to `bool` to match its definition.

Fix `check_convert_as_all_dims` and `check_convert_as_all_types` to
combine `check_*` results using the `&=` operator rather than the `+=`
operator. This change is consistent with the definition of
`check_vector_convert_modes`.

Fix `check_vector_convert_result` to use the `rte` or `rtz` conversion
functions when the `mode` is `rounding_mode::automatic`.

Signed-off-by: Michael Aziz <michael.aziz@intel.com>
@0x12CC 0x12CC requested a review from a team as a code owner July 21, 2023 14:49
tests/common/common_vec.h Outdated Show resolved Hide resolved
@0x12CC 0x12CC requested review from bader and gmlueck July 25, 2023 14:15
gmlueck added a commit to gmlueck/SYCL-Docs that referenced this pull request Jul 25, 2023
This change was prompted by a discussion in KhronosGroup/SYCL-CTS#756,
relating to a test of `vec::as`.  That test tried to use `vec::as` to
reinterpret a `sycl::vec<float, 3>` as `sycl::vec<int, 4>`, which is
legal according to the spec because they both have the same storage
size.  (This is because a `vec` of length 3 has the same storage size
as a `vec` of length 4.)  However, the semantics are unclear because
the content of the 4th element is undefined for a 3 element `vec`.

It seems reasonable to simply state that such a use of `vec::as` is not
allowed.  I think our intent was to allow `vec::as` to reinterpret only
the bits of defined elements in a `vec`, which is what this change
does.
Signed-off-by: Michael Aziz <michael.aziz@intel.com>
@bader bader requested a review from keryell July 26, 2023 00:41
@fraggamuffin
Copy link

another reviewer please

@bader bader merged commit ebf2625 into KhronosGroup:SYCL-2020 Aug 1, 2023
8 checks passed
@0x12CC 0x12CC deleted the vector_check branch August 1, 2023 15:31
keryell added a commit to KhronosGroup/SYCL-Docs that referenced this pull request Aug 17, 2023
This change was prompted by a discussion in KhronosGroup/SYCL-CTS#756, relating to a test of `vec::as`.  That test tried to use `vec::as` to reinterpret a `sycl::vec<float, 3>` as `sycl::vec<int, 4>`, which is legal according to the spec because they both have the same storage size.  (This is because a `vec` of length 3 has the same storage size as a `vec` of length 4.)  However, the semantics are unclear because the content of the 4th element is undefined for a 3 element `vec`.

It seems reasonable to simply state that such a use of `vec::as` is not allowed.  I think our intent was to allow `vec::as` to reinterpret only the bits of defined elements in a `vec`, which is what this change does.
keryell added a commit to KhronosGroup/SYCL-Docs that referenced this pull request Sep 10, 2024
This change was prompted by a discussion in KhronosGroup/SYCL-CTS#756, relating to a test of `vec::as`.  That test tried to use `vec::as` to reinterpret a `sycl::vec<float, 3>` as `sycl::vec<int, 4>`, which is legal according to the spec because they both have the same storage size.  (This is because a `vec` of length 3 has the same storage size as a `vec` of length 4.)  However, the semantics are unclear because the content of the 4th element is undefined for a 3 element `vec`.

It seems reasonable to simply state that such a use of `vec::as` is not allowed.  I think our intent was to allow `vec::as` to reinterpret only the bits of defined elements in a `vec`, which is what this change does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants