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

Added cl_half support for test_select #1617

Merged
merged 6 commits into from
Jun 27, 2023

Conversation

shajder
Copy link
Contributor

@shajder shajder commented Jan 9, 2023

According to issue description (select section):

#142

test_conformance/select/util_select.cpp Outdated Show resolved Hide resolved
test_conformance/select/test_select.h Show resolved Hide resolved
test_conformance/select/util_select.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried this PR on Codeplay's implementation and can see the new tests pass (and skip correctly against an implementation without cl_khr_fp16), but need to review the PR in more detail.

test_conformance/select/util_select.cpp Outdated Show resolved Hide resolved
@EwanC
Copy link
Contributor

EwanC commented Feb 24, 2023

Tried this PR on Codeplay's implementation and can see the new tests pass (and skip correctly against an implementation without cl_khr_fp16), but need to review the PR in more detail.

To follow up, I've looked through this again, and happy that how it adds half support is consistent with the other tests.

How/when to do the refactoring to make it nicer is another question.

@svenvh svenvh self-requested a review March 27, 2023 09:18
@shajder
Copy link
Contributor Author

shajder commented Jun 13, 2023

@EwanC due to the fact my project comes to an end and I can't see any progress around PR I am blocked with I decided to move on.

Except of corrections for above requests I've added optimization which seems to speed up the test ~18% at my machine. Moreover, I've replaced CL objects with specific wrappers.

@shajder shajder marked this pull request as ready for review June 13, 2023 12:51
EwanC
EwanC previously approved these changes Jun 13, 2023
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EwanC due to the fact my project comes to an end and I can't see any progress around #1665 I am blocked with I decided to move on.

Except of corrections for above requests I've added optimization which seems to speed up the test ~18% at my machine. Moreover, I've replaced CL objects with specific wrappers.

Had a look through this again, and happy with where we've got to with this change.

test_conformance/select/test_select.cpp Outdated Show resolved Hide resolved
svenvh
svenvh previously approved these changes Jun 14, 2023
@svenvh
Copy link
Member

svenvh commented Jun 14, 2023

LGTM, but please remove the excess semicolon that Ewan pointed out.

@shajder shajder dismissed stale reviews from svenvh and EwanC via e088029 June 14, 2023 12:58
svenvh
svenvh previously approved these changes Jun 14, 2023
EwanC
EwanC previously approved these changes Jun 14, 2023
bashbaug
bashbaug previously approved these changes Jun 22, 2023
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, aside from one minor comment.

I didn't do a super detailed code review but I did verify that the additions pass on our implementation and I did not see any change in coverage (aside from the added half tests). Thanks!

test_conformance/select/test_select.cpp Outdated Show resolved Hide resolved
@shajder shajder dismissed stale reviews from bashbaug, EwanC, and svenvh via e085eb4 June 22, 2023 07:52
@shajder
Copy link
Contributor Author

shajder commented Jun 23, 2023

Close/open to trigger CI

@shajder shajder closed this Jun 23, 2023
@shajder shajder reopened this Jun 23, 2023
@neiltrevett neiltrevett merged commit 60f025a into KhronosGroup:main Jun 27, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants