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

Add cl_khr_kernel_clock tests #1960

Merged
merged 6 commits into from
May 21, 2024

Conversation

AhmedAmraniAkdi
Copy link
Contributor

@AhmedAmraniAkdi AhmedAmraniAkdi commented May 1, 2024

Adds cl_khr_kernel_clock test.

Also fixes failure in the compiler defines for extension compiler subtest when cl_khr_kernel_clock is supported.

…xtensions array.

Add newline to CMakeLists.txt
Wrap everything under an anonymous namespace, remove extern keyword, use c++ raw strings for the ocl kernels, and remove underscore from CL_KHR_KERNEL_CLOCK_PROCS_H
if (buf == 1)
{
log_error(
"Sampling the clock returned bad values, time1 > time2.\n");
Copy link
Member

@svenvh svenvh May 8, 2024

Choose a reason for hiding this comment

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

Not sure if the extension provides a guarantee that time2 > time1? The description for clock_read_* says

an observer may see sampled values wrap around zero.

which would make the test fail spuriously if a wraparound happens between the time1 and time2 sampling points.

I tried to think of a more robust way to test the returned values, but I'm not even sure if we have a guarantee that time1 != time2? What do other reviewers think?

Copy link
Contributor

Choose a reason for hiding this comment

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

There was a bit of discussion on GitLab about this also. I agree there could be a problem here in theory, but these tests are currently matching the behavior for the Vulkan tests, see:

https://github.com/KhronosGroup/VK-GL-CTS/blob/main/external/vulkancts/modules/vulkan/shaderexecutor/vktShaderClockTests.cpp#L182

Since there doesn't seem to have been a problem with the Vulkan tests in practice so far, I'm inclined to keep these tests as-is, and if we end up needing something fancier we can cross that bridge when we come to it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for cross-checking, that sounds reasonable to me then.

@lakshmih
Copy link
Contributor

Reviewing

@bashbaug
Copy link
Contributor

Merging as discussed in the May 21st teleconference.

@bashbaug bashbaug merged commit cdf8d5e into KhronosGroup:main May 21, 2024
7 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.

4 participants