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

fix cvk_command_image_init for 3D images #612

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

rjodinchr
Copy link
Contributor

No description provided.

Copy link
Owner

@kpet kpet left a comment

Choose a reason for hiding this comment

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

This looks like an obvious fix which makes me suspicious as to why it wasn't done like that in the first place. Maybe the 1 was used for debugging and changing that back was missed when merging to main. At least we clearly don't have adequate test coverage :). How did you find it?

@rjodinchr
Copy link
Contributor Author

I find it working on samplerless_reads. It seems that this is almost the only test using this feature.

But it is even more complicated than that. Because the test is not comparing the value in the image on the host side. It is just comparing the values coming from a sampler read and a samplerless reads. When we do not initialise the image, we just have garbage values there. The same comes from the sampler read and the samplerless one.

But on AMD when you read with a sampler, all NaN are encoded as oxffc00000 whatever the real Nan value is (maybe 0x7fc00023 for example). Making the comparison fail.
Working on fixing the test for AMD, I finally found that it was due to this bug.

@rjodinchr
Copy link
Contributor Author

Note that #614 will add coverage for it in clvk.

@kpet
Copy link
Owner

kpet commented Oct 8, 2023

Ouch, that's a bit of a gap in the CTS. #614 will get us one test but the CTS should probably do more. I've created KhronosGroup/OpenCL-CTS#1823.

@kpet kpet merged commit d2c0cd0 into kpet:main Oct 8, 2023
@rjodinchr rjodinchr deleted the pr/fix-image-init-3d branch October 9, 2023 06:40
rjodinchr added a commit to rjodinchr/clvk that referenced this pull request Oct 11, 2023
After kpet#612 the cts started to regress on swiftshader.
It is because in the cts, the height and depth can be set to zero when
the dimension is not used.
kpet pushed a commit that referenced this pull request Oct 12, 2023
After #612 the cts started to regress on swiftshader.
It is because in the cts, the height and depth can be set to zero when
the dimension is not used.
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