-
Notifications
You must be signed in to change notification settings - Fork 731
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] Fix bitselect builtin for integer types #12598
Conversation
This regressed after intel#11956 as return type wasn't correctly converted from SPIR-V intrinsic back to SYCL types. This PR fixes that. In addition, I'm also adding tests for `sycl::select` builtin that was left unaffected only because we couldn't use SPIR-V intrinsic for its implementation.
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
// RUN: %{run} %t.out | ||
// RUN: %if preview-breaking-changes-supported %{ %{build} -fpreview-breaking-changes -o %t_preview.out %} | ||
// RUN: %if preview-breaking-changes-supported %{ %{run} %t_preview.out%} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Perhaps you could add a comment here to state the objective(s) of this test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not different from existing marray_relational.cpp
and vec_relational.cpp
and it never caused troubles to understand what they are doing... Are you asking about preview specifically? If yes, then it's a common idiom and we don't expect to document it in every file that uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I was talking about providing a broader overview of this test case. To my inexperienced eyes, it wasn't readily clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it matches how other builtins tests are set up, so I'm going to ignore it in scope of this PR.
sycl::vec<uint8_t, 2> c{0b1010, 0b1010}; | ||
sycl::vec<uint8_t, 2> r{0b0110, 0b1001}; | ||
|
||
auto BitSelect = [](auto... xs) { return sycl::bitselect(xs...); }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering do we have negative test cases for sycl::bitselect (with vector type inputs) to check if all the arguments to sycl::bitselect have same element types and the same vector length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm relying on CTS to catch every possible obscure corner case. Otherwise, the full testing for all the builtins would be taking way too much time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
Post commit failures on Intel Arc GPU:
|
This regressed after #11956 as return type wasn't correctly converted from SPIR-V intrinsic back to SYCL types. This PR fixes that.
In addition, I'm also adding tests for
sycl::select
builtin that was left unaffected only because we couldn't use SPIR-V intrinsic for its implementation.