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

[SYCL] Make mutable swizzle functions unusable on const vec #13026

Conversation

steffenlarsen
Copy link
Contributor

Swizzles should not expose mutating functions when the underlying vector
is const. This commit SFINAEs these out.

This is built on top of #13012.

In intel#12682 the mutating operators for
swizzles (+=, -=, ..., ++, --) were reverted to be members rather than
friends. Since swizzles mutate the underlying vec rather than themselves
these operators should take and return constant references instead,
which this commit implements.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Swizzles should not expose mutating functions when the underlying vector
is const. This commit SFINAEs these out.

This is built on top of intel#13012.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@gmlueck
Copy link
Contributor

gmlueck commented Mar 15, 2024

I didn't really review the code closely, but I ran some test cases. Everything looked good, except this one which won't compile:

const sycl::vec<int, 4> v4{0};
using type = decltype(v4.swizzle<0>())::value_type;

It seems like the __swizzled_vec__ type does not define the value_type member type. I think this should be an alias to the same type as element_type (which is int in this case).

@steffenlarsen
Copy link
Contributor Author

I didn't really review the code closely, but I ran some test cases. Everything looked good, except this one which won't compile:

const sycl::vec<int, 4> v4{0};
using type = decltype(v4.swizzle<0>())::value_type;

It seems like the __swizzled_vec__ type does not define the value_type member type. I think this should be an alias to the same type as element_type (which is int in this case).

Good find! Adding those can be done in separation, as I have done here: #13040

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen marked this pull request as ready for review June 27, 2024 16:24
@steffenlarsen steffenlarsen requested a review from a team as a code owner June 27, 2024 16:24
@steffenlarsen
Copy link
Contributor Author

KhronosGroup/SYCL-Docs#514 has been merged. This is ready for review! Keep in mind that it is based on #13012.

@steffenlarsen
Copy link
Contributor Author

Discussing this with Andrei, we agreed that the proposed variant was too intrusive on the signatures compared to the new spec definitions. Instead, I have moved the operators to new base-classes, which are in turn specialized to match the requirements of the various operators.

Note: I included the changes as part of the merge commit by accident.

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.

2 participants