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

Test cl_khr_command_buffer_mutable_dispatch #1481

Closed
21 tasks done
EwanC opened this issue Aug 31, 2022 · 9 comments
Closed
21 tasks done

Test cl_khr_command_buffer_mutable_dispatch #1481

EwanC opened this issue Aug 31, 2022 · 9 comments
Labels
missing-coverage mobica-backlog Issue approved by WG for Mobica to work on

Comments

@EwanC
Copy link
Contributor

EwanC commented Aug 31, 2022

Test plan for cl_khr_command_buffer_mutable_dispatch breaking down the areas which need to be covered.

1 Device Info Query

Verify the following queries can be performed successfully and return
defined values:

  • CL_DEVICE_MUTABLE_DISPATCH_CAPABILITIES_KHR

EDIT - Done in #1651

2 Mutable-commands

2.1 Mutable-dispatch

For each property in cl_mutable_dispatch_properties_khr reported by device query.

  • Create a command-buffer with CL_COMMAND_BUFFER_MUTABLE_KHR
  • Record clCommandNDRangeKernelKHR returning a cl_mutable_command_khr object
  • Enqueue command-buffer and verify
  • Set a new value for the mutable value, and enqueue again, verify result
  • Enqueue command-buffer again, without updating mutable-dispatch, and verify that new values have persisted.

2.1.1 Multiple updates

2.1.2 Simultaneous use

If a device supports simultaneous-use

  • Test updating mutable-dispatch objects of a command-buffer while a previous instance of the command-buffer is in flight.

2.1.3 cross-queue simultaneous-use

  • cross-queue simultaneous-use

EDIT: #1912

If a device supports simultaneous-use

Test updating mutable-dispatch objects of a command-buffer while a previous instance of the command-buffer is in flight to a different queue. See 1.3.3 from the cl_khr_comand_buffer test plan, but changing one of the command-buffer enqueues to the original queue.

2.2 clGetMutableCommandInfoKHR

Test each of the queries in clGetMutableCommandInfoKHR():

  • CL_MUTABLE_COMMAND_COMMAND_QUEUE_KHR
  • CL_MUTABLE_COMMAND_COMMAND_BUFFER_KHR
  • CL_MUTABLE_DISPATCH_PROPERTIES_ARRAY_KHR
  • CL_MUTABLE_DISPATCH_KERNEL_KHR
  • CL_MUTABLE_DISPATCH_DIMENSIONS_KHR
  • CL_MUTABLE_DISPATCH_GLOBAL_WORK_OFFSET_KHR
  • CL_MUTABLE_DISPATCH_GLOBAL_WORK_SIZE_KHR
  • CL_MUTABLE_DISPATCH_LOCAL_WORK_SIZE_KHR
  • CL_MUTABLE_COMMAND_COMMAND_TYPE_KHR

Edit - Done in #1651

@EwanC EwanC added the mobica-triage Issue proposed for addition to Mobica's backlog (needs WG approval) label Nov 15, 2022
@neiltrevett neiltrevett added mobica-backlog Issue approved by WG for Mobica to work on and removed mobica-triage Issue proposed for addition to Mobica's backlog (needs WG approval) labels Feb 14, 2023
@EwanC
Copy link
Contributor Author

EwanC commented Jul 6, 2023

Now that the oneAPI Construction Kit is open source, i'll highlight that it has unit tests for this extension that could be used for inspiration to fill in the CTS gaps, although it focuses on kernel argument update. https://github.com/codeplaysoftware/oneapi-construction-kit/tree/main/source/cl/test/UnitCL/source/cl_khr_command_buffer_mutable_dispatch

In particular, to the plan above:

Test using clUpdateMutableCommandsKHR() in a way which updates more than one command-handle in the same call.

https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/source/cl/test/UnitCL/source/cl_khr_command_buffer_mutable_dispatch/clUpdateMutableCommandsKHR.cpp#L1119

Test calling clUpdateMutableCommandsKHR() several times on the same command-handle and make sure that the most recent value persists.

https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/source/cl/test/UnitCL/source/cl_khr_command_buffer_mutable_dispatch/clUpdateMutableCommandsKHR.cpp#L764

Test update being called multiple times on the same command before an enqueue, but with different arguments. Verify that all the combined updates are made.

https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/source/cl/test/UnitCL/source/cl_khr_command_buffer_mutable_dispatch/clUpdateMutableCommandsKHR.cpp#L674

@EwanC
Copy link
Contributor Author

EwanC commented Feb 14, 2024

Noting that the mutable dispatch asserts from PR KhronosGroup/OpenCL-Docs#992 also need CTS testing as described in #1897

@shajder
Copy link
Contributor

shajder commented Feb 26, 2024

hello @EwanC, I am begining my work on finishing this issue and after initial review I am a bit concerned about points 2.1.2/2.1.3. Work plan does not mention anything about out-of-order command queues, meanwhile there are already two (?) tests added addressing 2.1.2 which uses out-of-order queue, I am guessing inspired by cl_khr_command_queue preceding tests. Do we want to leave it as is or get rid of out-of-order queues ? Do we want the new test addressing 2.1.3 to use out-of-order command queue ? Thanks

@EwanC
Copy link
Contributor Author

EwanC commented Feb 26, 2024

Hi @shajder, that's great you are able to pick this up again. That's a good point about out-of-order queues, device support for that is conditional on CL_COMMAND_BUFFER_CAPABILITY_OUT_OF_ORDER_KHR but we still want to test devices which don't support that optional capability in the 2.1.2 / 2.1.3 scenarios. So could you add tests that only use in-order queues for these scenarios? Realize that doing 2.1.2 again for an in-order queue might involve some duplication of the original test unless it can be refactored to also support in-order.

Regarding getting rid of the out-of-order queues, using an out-of-order queue is a valid use of the extension so I'm a bit reluctant to reduce coverage from a valid test.

@shajder
Copy link
Contributor

shajder commented Feb 26, 2024

... So could you add tests that only use in-order queues for these scenarios? Realize that doing 2.1.2 again for an in-order queue might involve some duplication of the original test unless it can be refactored to also support in-order.

make sense, I will refactor code to handle all mentioned cases

Regarding getting rid of the out-of-order queues, using an out-of-order queue is a valid use of the extension so I'm a bit reluctant to reduce coverage from a valid test.

understood, I will keep out-of-order test cases along with new in-order ones

@shajder
Copy link
Contributor

shajder commented Feb 27, 2024

@EwanC one more concern, in tests mutable_dispatch_global_size/mutable_dispatch_local_size/mutable_dispatch_global_offset argument cl_ndrange_kernel_command_properties_khr is not passed to clCommandNDRangeKernelKHR. Is this a mistake? According to spec (eg. CL_MUTABLE_DISPATCH_GLOBAL_OFFSET_KHR):

... Determines whether the global_work_offset of kernel execution can be modified after recording. If set, the global_work_offset of the kernel execution can be changed with clUpdateMutableCommandsKHR using the cl_mutable_dispatch_config_khr field of the mutable_config parameter. Otherwise, the global_work_offset cannot be modified.

@EwanC
Copy link
Contributor Author

EwanC commented Feb 27, 2024

@EwanC one more concern, in tests mutable_dispatch_global_size/mutable_dispatch_local_size/mutable_dispatch_global_offset argument cl_ndrange_kernel_command_properties_khr is not passed to clCommandNDRangeKernelKHR. Is this a mistake? According to spec (eg. CL_MUTABLE_DISPATCH_GLOBAL_OFFSET_KHR):

... Determines whether the global_work_offset of kernel execution can be modified after recording. If set, the global_work_offset of the kernel execution can be changed with clUpdateMutableCommandsKHR using the cl_mutable_dispatch_config_khr field of the mutable_config parameter. Otherwise, the global_work_offset cannot be modified.

Another good question, there is the language in the spec for cl_mutable_dispatch_fields_khr that

If CL_MUTABLE_DISPATCH_UPDATABLE_FIELDS_KHR is not specified then it defaults to the value returned by the CL_DEVICE_MUTABLE_DISPATCH_CAPABILITIES_KHR device query.

Therefore I think the tests are correct by that default wording since CL_DEVICE_MUTABLE_DISPATCH_CAPABILITIES_KHR is checked in Skip(), however it does also leave a gap in testing about whether implementation correctly handle cl_ndrange_kernel_command_properties_khr if it is passed here. So I would say we could do something like improve the mutable_dispatch_global_size/mutable_dispatch_local_size/mutable_dispatch_global_offset tests to have a second run (or use a second kernel) where the clCommandNDRangeKernelKHR kernel is created with cl_ndrange_kernel_command_properties_khr

shajder added a commit to shajder/OpenCL-CTS that referenced this issue Mar 5, 2024
shajder added a commit to shajder/OpenCL-CTS that referenced this issue Mar 7, 2024
shajder added a commit to shajder/OpenCL-CTS that referenced this issue Mar 11, 2024
-cross queue simultaneous use
-in-order queue with simultaneous use

According to issue description KhronosGroup#1481
@shajder
Copy link
Contributor

shajder commented Mar 11, 2024

Realize that doing 2.1.2 again for an in-order queue might involve some duplication of the original test unless it can be refactored to also support in-order.

Added #1912 along with two test cases:
-mutable dispatch with simultaneous use in in-order queue
-mutable dispatch with simultaneous use in cross queue scenario

bashbaug pushed a commit that referenced this issue Mar 12, 2024
* Added command buffer with full mutable dispatch test

According to #1481 issue description, point 2.1

* Corrected the test to handle all available mutable properties

According to #1481 issue description, point 2.1
shajder added a commit to shajder/OpenCL-CTS that referenced this issue Mar 15, 2024
shajder added a commit to shajder/OpenCL-CTS that referenced this issue Mar 18, 2024
shajder added a commit to shajder/OpenCL-CTS that referenced this issue Mar 19, 2024
neiltrevett pushed a commit that referenced this issue Mar 19, 2024
* Added new tests for simultaneous use with mutable dispatch

-cross queue simultaneous use
-in-order queue with simultaneous use

According to issue description #1481

* Several corrections applied:

-reordered Skip conditions to check valid simultaneous_use_support flag
-removed unnecessary SetUpKernel call
-initialize kernel and memory buffers from BasicCommandBufferTest
instead BasicMutableCommandBufferTest

* Corrections for command buffer creation to request simultaneous property
neiltrevett pushed a commit that referenced this issue Mar 19, 2024
…date (#1919)

* Added new test to cover multiple comands dispatch in one enqueued update

According to issue description #1481

* clang format correction

* Few minor corrections

* cosmetic corrections
neiltrevett pushed a commit that referenced this issue Mar 26, 2024
* Added test to cover overwritten update of mutable parameters

According to issue description #1481

* cosmetic corrections

* Corrections due to code review
neiltrevett pushed a commit that referenced this issue Mar 26, 2024
* Added test to cover iterative mutable argument update

According to issue description #1481

* cosmetic correction

* clang format fix

* Corrections due to code review
@EwanC
Copy link
Contributor Author

EwanC commented Mar 26, 2024

Closing this issue as all the items in the test plan have been covered with a merged CTS PR. New issues can be open to track any future CTS gaps in cl_khr_command_buffer_mutable_dispatch

@EwanC EwanC closed this as completed Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing-coverage mobica-backlog Issue approved by WG for Mobica to work on
Projects
None yet
Development

No branches or pull requests

3 participants