fix: synchronize resubmission of the same command buffer #18566

EuphoricThinking · 2025-05-20T12:54:13Z

added a test for in-order queues with command buffers

pbalcer · 2025-05-20T13:10:20Z

unified-runtime/source/adapters/level_zero/v2/command_list_manager.cpp

@@ -158,19 +158,14 @@ ur_result_t ur_command_list_manager::appendRegionCopyUnlocked(
 }

 wait_list_view ur_command_list_manager::getWaitListView(
-    const ur_event_handle_t *phWaitEvents, uint32_t numWaitEvents,
-    ur_event_handle_t additionalWaitEvent) {
+    const ur_event_handle_t *phWaitEvents, uint32_t numWaitEvents) {


@staniewzki is using this API in https://github.com/intel/llvm/pull/18179/files#diff-57b5ceadf73f501ea8d07f9837583b61b45ca22129f3aacdeda1fb0b582a9990R735-R736.

Xewar313 · 2025-05-20T13:20:37Z

unified-runtime/test/conformance/exp_command_buffer/fixtures.h

    ur_exp_command_buffer_desc_t desc{UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC,
-                                      nullptr, false, false, false};
+                                      nullptr, false, true /* WAS: false */, false};


Why change the buffer to in order?

In order to extend the time of the buffer execution, but as @pbalcer mentioned, I'll revert this to out-of-order

Xewar313 · 2025-05-20T13:20:59Z

unified-runtime/test/conformance/exp_command_buffer/fixtures.h

@@ -112,6 +113,7 @@ struct urCommandBufferExpExecutionTest : uur::urKernelExecutionTest {
  }

  ur_exp_command_buffer_handle_t cmd_buf_handle = nullptr;
+  ur_exp_command_buffer_handle_t cmd_buf_in_order = nullptr;


Where is it used?

In the interest of time, and knowing that right now there's no difference between the two, I suggest not adding in order cmd buf here for now, and just testing out of order for now (both should exhibit the issue we are trying to solve).

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

pbalcer · 2025-05-22T09:40:06Z

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

@@ -132,3 +154,25 @@ TEST_P(urEnqueueCommandBufferExpTest, SerializeOutofOrderQueue) {
    ASSERT_EQ(reference, Output[i]);
  }
 }
+
+TEST_P(urEnqueueCommandBufferExpTest, SerializeInOrderQueue) {


this test is nearly identical to SerializeOutofOrderQueue. Parameters are just different. You could use UUR_DEVICE_TEST_SUITE_WITH_PARAM or just create a function with the body of the test. Whatever is simpler (though using the macro is probably more idiomatic).

I've used the macro and introduced a new fixture for parametrized tests.

pbalcer

lgtm, just a few nits.

pbalcer · 2025-05-29T12:28:12Z

unified-runtime/source/adapters/level_zero/v2/queue_immediate_in_order.cpp

+  if (executionEvent != nullptr) {
+    ZE2UR_CALL(zeEventHostSynchronize, (executionEvent->getZeEvent(), UINT64_MAX));
+  }
+
  UR_CALL(enqueueGenericCommandListsExp(
      1, &commandBufferCommandList, phEvent, numEventsInWaitList,
      phEventWaitList, UR_COMMAND_ENQUEUE_COMMAND_BUFFER_EXP, executionEvent));


Suggested change

phEventWaitList, UR_COMMAND_ENQUEUE_COMMAND_BUFFER_EXP, executionEvent));

phEventWaitList, UR_COMMAND_ENQUEUE_COMMAND_BUFFER_EXP, nullptr));

We can change this to nullptr, since we know for sure this event is already signaled (completed). No need to add it to the waitlist of the command.

pbalcer · 2025-05-29T12:30:19Z

unified-runtime/test/conformance/exp_command_buffer/fixtures.h

@@ -98,7 +98,7 @@ struct urCommandBufferExpExecutionTest : uur::urKernelExecutionTest {
    UUR_RETURN_ON_FATAL_FAILURE(checkCommandBufferSupport(device));

    ur_exp_command_buffer_desc_t desc{UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC,
-                                      nullptr, false, false, false};
+                                      nullptr, false, false /* WAS: false */, false};


Suggested change

nullptr, false, false /* WAS: false */, false};

nullptr, false, false, false};

pbalcer · 2025-05-29T12:34:38Z

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

  }

-  ur_queue_handle_t in_order_queue = nullptr;
-  ur_queue_handle_t out_of_order_queue = nullptr;
+  ur_queue_handle_t in_or_out_of_order_queue = nullptr;


just call it queue.

There is already a queue in the parent class (urQueueTestWithParam). This queue is used in SerializeAcrossQueues

Can't we use the one from the parent class then? I think we can just modify queue_properties BEFORE calling parent SetUp (UUR_RETURN_ON_FATAL_FAILURE( urCommandBufferExpExecutionTestWithParam::SetUp());) to make it in-order or out-of-order

This reverts commit 80a1ff0.

This reverts commit 99389f8.

add new test fixture test serialization of the command buffer execution in an in-order queue

pbalcer · 2025-05-29T13:28:23Z

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

+
+  static constexpr int num_copy_buffers = 10;
+  static constexpr int buffer_size = 512;
+  int *dst_buffers[num_copy_buffers];


OpenCL tests are failing:

/__w/llvm/llvm/unified-runtime/test/conformance/exp_command_buffer/fixtures.h:25: Skipped EXP command-buffer feature is not supported. /__w/llvm/llvm/unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp:93: Failure Expected equality of these values: uur::Result(UR_RESULT_SUCCESS) Which is: UR_RESULT_SUCCESS uur::Result(urUSMFree(context, src_buffers[i])) Which is: UR_RESULT_ERROR_INVALID_VALUE

You might need to zero initialize these buffers (add = {} to both arrays);

EwanC · 2025-05-29T13:57:20Z

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

+                                      (void **)&(src_buffers[i])));
+
+      ASSERT_SUCCESS(urEnqueueUSMMemcpy(
+          in_or_out_of_order_queue, false, src_buffers[i], temp_val.data(),


It's odd submitting this to in_or_out_of_order_queue when the calls before and after are to queue.

Does it look better now? c:

EwanC · 2025-05-29T13:58:15Z

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

@@ -45,6 +39,22 @@ struct urEnqueueCommandBufferExpTest
    ASSERT_SUCCESS(urEnqueueUSMFill(queue, device_ptr, sizeof(zero_pattern),
                                    &zero_pattern, allocation_size, 0, nullptr,
                                    nullptr));
+
+    std::vector<int> temp_val(buffer_size, 3);


Preference for using urEnqueueUSMFill on line 53 than initializing then copying a host vector of a constant value.

EwanC · 2025-05-29T13:59:32Z

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

+  static constexpr int num_copy_buffers = 10;
+  static constexpr int buffer_size = 512;
+  int *dst_buffers[num_copy_buffers];
+  int *src_buffers[num_copy_buffers];


preference for fixed sized type like int32_t, whereas int is system dependent.

EwanC · 2025-05-29T14:04:10Z

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp

-  ur_queue_handle_t in_order_queue = nullptr;
-  ur_queue_handle_t out_of_order_queue = nullptr;
+  ur_queue_handle_t in_or_out_of_order_queue = nullptr;
+  ur_queue_flags_t queue_type;


Does this need to be a class member variable or could we just define it on line 28?

No, once upon a time I've thought this looks better, but these times are gone.

synchronization implemented as the host thread waiting for an event to be signalled after the completion of the command buffer execution

EuphoricThinking temporarily deployed to WindowsCILock May 20, 2025 12:54 — with GitHub Actions Inactive

pbalcer reviewed May 20, 2025

View reviewed changes

EuphoricThinking temporarily deployed to WindowsCILock May 20, 2025 13:17 — with GitHub Actions Inactive

Xewar313 reviewed May 20, 2025

View reviewed changes

pbalcer mentioned this pull request May 22, 2025

[UR][L0 v2] urEnqueueCommandBufferExpTest.SerializeAcrossQueues flaky on PVC #18610

Open

EwanC reviewed May 22, 2025

View reviewed changes

unified-runtime/test/conformance/exp_command_buffer/enqueue.cpp Outdated Show resolved Hide resolved

pbalcer reviewed May 22, 2025

View reviewed changes

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from 0cc9c9e to f9c9545 Compare May 29, 2025 12:13

EuphoricThinking had a problem deploying to WindowsCILock May 29, 2025 12:13 — with GitHub Actions Error

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from f9c9545 to d7bff7e Compare May 29, 2025 12:16

EuphoricThinking had a problem deploying to WindowsCILock May 29, 2025 12:16 — with GitHub Actions Error

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from d7bff7e to b4f49fd Compare May 29, 2025 12:33

EuphoricThinking had a problem deploying to WindowsCILock May 29, 2025 12:34 — with GitHub Actions Error

pbalcer approved these changes May 29, 2025

View reviewed changes

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from b4f49fd to 277615b Compare May 29, 2025 12:37

EuphoricThinking had a problem deploying to WindowsCILock May 29, 2025 12:38 — with GitHub Actions Error

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from 277615b to 0752cac Compare May 29, 2025 12:48

EuphoricThinking had a problem deploying to WindowsCILock May 29, 2025 12:49 — with GitHub Actions Error

EuphoricThinking marked this pull request as ready for review May 29, 2025 12:51

EuphoricThinking requested review from a team as code owners May 29, 2025 12:51

EuphoricThinking requested a review from fabiomestre May 29, 2025 12:51

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from 0752cac to e2b3ff4 Compare May 29, 2025 13:01

EuphoricThinking temporarily deployed to WindowsCILock May 29, 2025 13:02 — with GitHub Actions Inactive

EuphoricThinking added 3 commits May 29, 2025 13:08

Revert "[UR][Graph] Disable flaky CTS test on L0 v2 (intel#18636)"

83561a8

This reverts commit 80a1ff0.

Revert "[UR][Graph] Disable flaky CTS test on L0 v2 (intel#18611)"

5c30e32

This reverts commit 99389f8.

add a test for serialization of command buffers

a8b1b8f

add new test fixture test serialization of the command buffer execution in an in-order queue

EuphoricThinking temporarily deployed to WindowsCILock May 29, 2025 13:21 — with GitHub Actions Inactive

pbalcer reviewed May 29, 2025

View reviewed changes

EwanC reviewed May 29, 2025

View reviewed changes

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from e2b3ff4 to 2aac6c4 Compare May 30, 2025 01:33

EuphoricThinking had a problem deploying to WindowsCILock May 30, 2025 01:34 — with GitHub Actions Error

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from 2aac6c4 to cb047e5 Compare May 30, 2025 01:42

EuphoricThinking had a problem deploying to WindowsCILock May 30, 2025 01:43 — with GitHub Actions Error

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from cb047e5 to 49dfd0a Compare May 30, 2025 01:55

EuphoricThinking had a problem deploying to WindowsCILock May 30, 2025 01:56 — with GitHub Actions Error

EuphoricThinking force-pushed the serialize_cmdbuff_pr branch from 49dfd0a to 232146b Compare May 30, 2025 02:14

EuphoricThinking temporarily deployed to WindowsCILock May 30, 2025 02:15 — with GitHub Actions Inactive

EuphoricThinking temporarily deployed to WindowsCILock May 30, 2025 02:38 — with GitHub Actions Inactive

fix: synchronize resubmission of the same command buffer

232146b

synchronization implemented as the host thread waiting for an event to be signalled after the completion of the command buffer execution

	phEventWaitList, UR_COMMAND_ENQUEUE_COMMAND_BUFFER_EXP, executionEvent));
	phEventWaitList, UR_COMMAND_ENQUEUE_COMMAND_BUFFER_EXP, nullptr));

	nullptr, false, false /* WAS: false */, false};
	nullptr, false, false, false};

fix: synchronize resubmission of the same command buffer #18566

Are you sure you want to change the base?

fix: synchronize resubmission of the same command buffer #18566

Conversation

EuphoricThinking commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbalcer May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbalcer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EuphoricThinking May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pbalcer May 22, 2025 •

edited

Loading

EuphoricThinking May 29, 2025 •

edited

Loading