-
Notifications
You must be signed in to change notification settings - Fork 68
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
Remove underspecified vector_t #676
Open
gmlueck
wants to merge
1
commit into
KhronosGroup:main
Choose a base branch
from
gmlueck:gmlueck/vec-vector-t
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains 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 change 9 of 9 that fix problems with the specification of the `vec` class. An implementation that follows the existing specification would not accept common code patterns and would not pass the CTS. None of the existing implementations actually follow the existing specification. This change removes the `vector_t` type alias from the `vec` class and also removes the conversions to and from this type. The intention of this type was to provide a conversion between `vec` and a compiler-specific builtin vector type. However, these conversion also lead to ambiguity errors in common code patterns such as: ``` sycl::vec<sycl::half, 1> h1; sycl::half h; h = h1; ``` Prior to this change, the last line could result in an ambiguity error from the compiler between: * Convert `vec<half,1>` -> `half`, call `half::operator=(const half&)` * Convert `vec<half,1>` -> `_Float16`, convert `_Float16` -> `float`, call `half::operator=(float)` What's worse is that the exact type alias of `vector_t` is implementation-defined, so these ambiguity errors could be different for each SYCL implementation. During the WG meeting, we learned that the two major SYCL implementations (DPC++ and AdaptiveCpp) define `vector_t` differently even though they are both based on clang. Given this lack of commonality, the WG decided to remove `vector_t` entirely from the spec. Implementations can decide for themselves whether to provide a compiler-specific conversion like this. These changes correspond to slides 29 - 31 of the presentation that was discussed in the WG meetings.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jan 6, 2025
* Don't use `sycl::vec::vector_t`, as it is planned to be removed from the SYCL 2020 (KhronosGroup/SYCL-Docs#676). Note that this implementation is NOT required to use it, so this PR can be merged before the specification change. * Use `ext_vector_type`-based optimized implementation whenever it's available and not on device only.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jan 6, 2025
* Don't use `sycl::vec::vector_t`, as it is planned to be removed from the SYCL 2020 (KhronosGroup/SYCL-Docs#676). Note that this implementation is NOT required to use it, so this PR can be merged before the specification change. * Use `ext_vector_type`-based optimized implementation whenever it's available and not on device only.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jan 6, 2025
* `vector_t` is expected to be removed in KhronosGroup/SYCL-Docs#676 * we aren't required to use it here * `operator vector_t`/`vec(vector_t)` are implemented as a simple `bit_cast` anyway, can use it explicitly as well.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jan 6, 2025
* `vector_t` is expected to be removed in KhronosGroup/SYCL-Docs#676 * We already create `OpenCLVec[TR]`, can use them directly
aelovikov-intel
added a commit
to intel/llvm
that referenced
this pull request
Jan 7, 2025
…6532) * `vector_t` is expected to be removed in KhronosGroup/SYCL-Docs#676 * We already create `OpenCLVec[TR]`, can use them directly
aelovikov-intel
added a commit
to intel/llvm
that referenced
this pull request
Jan 7, 2025
* `vector_t` is expected to be removed in KhronosGroup/SYCL-Docs#676 * we aren't required to use it here * `operator vector_t`/`vec(vector_t)` are implemented as a simple `bit_cast` anyway, can use it explicitly as well.
steffenlarsen
pushed a commit
to intel/llvm
that referenced
this pull request
Jan 8, 2025
* Don't use `sycl::vec::vector_t`, as it is planned to be removed from the SYCL 2020 (KhronosGroup/SYCL-Docs#676). Note that this implementation is NOT required to use it, so this PR can be merged before the specification change. * Use `ext_vector_type`-based optimized implementation whenever it's available and not on device only.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jan 8, 2025
* Don't use `sycl::vec::vector_t`, as it is planned to be removed from the SYCL 2020 (KhronosGroup/SYCL-Docs#676). Note that this implementation is NOT required to use it, so this PR can be merged before the specification change. * Use `ext_vector_type`-based optimized implementation whenever it's available and not on device only. This is a recommit of intel#16529 with an additional `#if __clang_major__ >= 20` guard around `static_assert` on the expression that wasn't constant in clang-19.
aelovikov-intel
added a commit
to intel/llvm
that referenced
this pull request
Jan 8, 2025
* Don't use `sycl::vec::vector_t`, as it is planned to be removed from the SYCL 2020 (KhronosGroup/SYCL-Docs#676). Note that this implementation is NOT required to use it, so this PR can be merged before the specification change. * Use `ext_vector_type`-based optimized implementation whenever it's available and not on device only. This is a recommit of #16529 with an additional `#if __clang_major__ >= 20` guard around `static_assert` on the expression that wasn't constant in clang-19.
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 change 9 of 9 that fix problems with the specification of the
vec
class. An implementation that follows the existing specification would not accept common code patterns and would not pass the CTS. None of the existing implementations actually follow the existing specification.This change removes the
vector_t
type alias from thevec
class and also removes the conversions to and from this type. The intention of this type was to provide a conversion betweenvec
and a compiler-specific builtin vector type. However, these conversion also lead to ambiguity errors in common code patterns such as:Prior to this change, the last line could result in an ambiguity error from the compiler between:
vec<half,1>
->half
, callhalf::operator=(const half&)
vec<half,1>
->_Float16
, convert_Float16
->float
, callhalf::operator=(float)
What's worse is that the exact type alias of
vector_t
is implementation-defined, so these ambiguity errors could be different for each SYCL implementation.During the WG meeting, we learned that the two major SYCL implementations (DPC++ and AdaptiveCpp) define
vector_t
differently even though they are both based on clang. Given this lack of commonality, the WG decided to removevector_t
entirely from the spec. Implementations can decide for themselves whether to provide a compiler-specific conversion like this.These changes correspond to slides 29 - 31 of the presentation that was discussed in the WG meetings.