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

Removed usage of half types in CTS test #2079

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kamil-goras-mobica
Copy link
Contributor

@kamil-goras-mobica kamil-goras-mobica changed the title Removed usage of half types in CTS test #1982 Removed usage of half types in CTS test Sep 11, 2024
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.

This isn't the correct fix - sorry for poorly explaining what needs to be done!

We want to keep the testing for CL_HALF_FLOAT. In other words, we shouldn't lose any test covereage after making these changes.

We want to remove the dependency on cl_khr_fp16 for these tests though, so the tests that currently require cl_khr_fp16 (which are used to test CL_HALF_FLOAT) need to be modified.

See the discussion here: #1974

One possible solution would be to eliminate all usage pointers to the half4 type by switching to pointers to the half type instead. This should allow removal of cl_khr_fp16, because pointers to the half type used by the vload_half and vstore_half functions is in core OpenCL.

A better (but more involved) solution is to eliminate all usage of half4, vload_half, and vstore_half, period, and switch to float reference data instead.

Let's chat more tomorrow if you have questions - thanks!

@bashbaug
Copy link
Contributor

I'm going to remove the "focused review" label until updates have been made.

@kamil-goras-mobica
Copy link
Contributor Author

@bashbaug I created corrections in separate PR: #2147

@bashbaug
Copy link
Contributor

bashbaug commented Dec 1, 2024

I think this can be closed now, especially since #2147 is merged?

@kamil-goras-mobica
Copy link
Contributor Author

@bashbaug Yes, this can be closed. Should I do it?

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.

3 participants