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

Array based clUpdateMutableCommandsKHR changes #1984

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Jun 19, 2024

CTS changes to reflect the spec changes merged in KhronosGroup/OpenCL-Docs#1045 and requires header updates from KhronosGroup/OpenCL-Headers#245

Tested using OCK implementation from uxlfoundation/oneapi-construction-kit#501

EwanC added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jun 19, 2024
Implementation change to prototype specification changes proposed
in KhronosGroup/OpenCL-Docs#1041

Uses OpenCL changes from
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
* KhronosGroup/OpenCL-CTS#1984
EwanC added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jul 19, 2024
Implementation change to prototype specification changes proposed
in KhronosGroup/OpenCL-Docs#1041

Uses OpenCL changes from
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
* KhronosGroup/OpenCL-CTS#1984
EwanC added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jul 19, 2024
Implementation change to prototype specification changes proposed
in KhronosGroup/OpenCL-Docs#1041

Uses OpenCL changes from
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
* KhronosGroup/OpenCL-CTS#1984
@EwanC EwanC marked this pull request as ready for review July 19, 2024 12:14
EwanC added a commit to EwanC/oneapi-construction-kit that referenced this pull request Jul 19, 2024
Implementation change to prototype specification changes proposed
in KhronosGroup/OpenCL-Docs#1041

Uses OpenCL changes from
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
* KhronosGroup/OpenCL-CTS#1984
@EwanC EwanC changed the title Draft array based clUpdateMutableCommandsKHR changes Array based clUpdateMutableCommandsKHR changes Sep 3, 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.

I don't have an easy way to test these changes (unless I pull and build the latest OCK) but they all look reasonable.

I think it's a good proof-point that the versioned extension headers work, but for the CTS tests I think we've generally been OK tracking the latest headers. Since we don't regularly test (or even build) with older headers I think the older code would eventually break anyhow.

I think it would be a good idea to skip these tests for older versions of the extension though, otherwise they'll likely crash unpredictably if the newer functions run on an older implementation. Could this be added?

CTS change to reflect proposed change from KhronosGroup/OpenCL-Docs#1041
Taking advantage of the extension version macros to avoid
breaking existing implementations immediately.

This reflects PRs:
* KhronosGroup/OpenCL-Docs#1045
* KhronosGroup/OpenCL-Headers#245
@EwanC
Copy link
Contributor Author

EwanC commented Sep 5, 2024

I don't have an easy way to test these changes (unless I pull and build the latest OCK) but they all look reasonable.

I think it's a good proof-point that the versioned extension headers work, but for the CTS tests I think we've generally been OK tracking the latest headers. Since we don't regularly test (or even build) with older headers I think the older code would eventually break anyhow.

I think it would be a good idea to skip these tests for older versions of the extension though, otherwise they'll likely crash unpredictably if the newer functions run on an older implementation. Could this be added?

I've removed the #ifdefs so this PR will require the matching headers to work. Also added in a CL_DEVICE_EXTENSIONS_WITH_VERSION runtime check and skipped testing when a device doesn't support at least 0.9.2 of mutable-dispatch. Although devices which don't support that query will still run into issues

@bashbaug
Copy link
Contributor

bashbaug commented Sep 6, 2024

Merging as discussed in the September 3rd teleconference + email.

@bashbaug bashbaug merged commit 0bdc5d0 into KhronosGroup:main Sep 6, 2024
5 of 7 checks passed
bashbaug added a commit that referenced this pull request Sep 17, 2024
fixes a few minor issues introduced by #1984.

edit: Tested with the updated command buffer emulation layer on a device
that supports out-of-order queues and SVM and all of the
`test_cl_khr_mutable_dispatch` tests pass.
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.

2 participants