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

Extend testing of CL_UNORM_INT_101010_2 #2031

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

gorazd-sumkovski-arm
Copy link
Contributor

All existing tests in test_image_streams, that are capable of testing image formats using the CL_UNORM_INT_101010_2 data type, now do so.

@CLAassistant
Copy link

CLAassistant commented Aug 1, 2024

CLA assistant check
All committers have signed the CLA.

All existing tests in `test_image_streams`, that are capable of testing
image formats using the `CL_UNORM_INT_101010_2` data type, now do so.
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.

I did a search for CL_UNORM_INT_101010, thinking that these are places that likely need to be updated for CL_UNORM_INT_101010_2. Can you check that these don't need updates?

  1. get_32_bit_image_format()
  2. compare_scanlines() - this one is probably OK.
  3. get_format_max_int()
  4. get_format_min_int()
  5. read_image_pixel()
  6. get_pixel_bytes()

There are a few others in other test suites also, but maybe those are intended for another PR.

@gorazd-sumkovski-arm
Copy link
Contributor Author

  1. get_32_bit_image_format() - Added (although this function isn't used anywhere).
  2. compare_scanlines() - As you suggested, no need for a special case because all 32 bits in CL_UNORM_INT_101010_2 are used.
  3. get_format_max_int() - Added.
  4. get_format_min_int() - Added.
  5. read_image_pixel<T>() - Already present in read_image_pixel_float(). Seems like read_image_pixel<T>() is used for int and uint while float has a separate function. As a side note, it looks like the cases for 101010, 555 and 565 in read_image_pixel<T>() are wrong, they are missing the normalisation divisions.
  6. get_pixel_bytes() - Added.

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.

I'd be surprised if there weren't a few more places that need updates for 101010_2, but I didn't find anymore cases that are obviously missing. Thanks!

@kpet
Copy link
Contributor

kpet commented Sep 16, 2024

Any objections to merging this PR ahead of the next teleconference? It is blocking the addition of testing for the 2_101010 format added by KhronosGroup/OpenCL-Docs#1223.

@bashbaug
Copy link
Contributor

I'm going to go ahead and merge this. Since these are purely additions I think the risk of problems for implementations that do not support the 101010_2 format is low. If we find problems we can always revert it.

@lakshmih FYI.

@bashbaug bashbaug merged commit bcfd8f8 into KhronosGroup:main Sep 16, 2024
7 checks passed
@kpet
Copy link
Contributor

kpet commented Sep 16, 2024

Thanks!

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.

4 participants