[UR] Make command-buffer creation descriptor mandatory (#17058)

EwanC · web-flow · commit 69941b863470 · 2025-02-20T10:03:14.000+01:00
As discussed in oneapi-src/unified-runtime#2670 (comment) the `pCommandBufferDesc` parameter to `urCommandBufferCreateExp` is optional. However, the UR spec doesn't state what the configuration of the created command-buffer is when this isn't passed, and being optional is also inconsistent with the description parameters to urSamplerCreate & urMemImageCreate which are not optional. This PR updates the descriptor parameter to command-buffer creation to be mandatory to address these concerns. Closes oneapi-src/unified-runtime#2673 **Note**: This UR patch was previously approved and ready-to-merge in oneapi-src/unified-runtime#2676 prior to the repo move
diff --git a/unified-runtime/include/ur_api.h b/unified-runtime/include/ur_api.h
@@ -10111,6 +10111,7 @@ typedef struct ur_exp_command_buffer_command_handle_t_
 ///         + `NULL == hContext`
 ///         + `NULL == hDevice`
 ///     - ::UR_RESULT_ERROR_INVALID_NULL_POINTER
+///         + `NULL == pCommandBufferDesc`
 ///         + `NULL == phCommandBuffer`
 ///     - ::UR_RESULT_ERROR_INVALID_CONTEXT
 ///     - ::UR_RESULT_ERROR_INVALID_DEVICE
@@ -10125,7 +10126,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext,
     /// [in] Handle of the device object.
     ur_device_handle_t hDevice,
-    /// [in][optional] command-buffer descriptor.
+    /// [in] Command-buffer descriptor.
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     /// [out][alloc] Pointer to command-Buffer handle.
     ur_exp_command_buffer_handle_t *phCommandBuffer);
diff --git a/unified-runtime/scripts/core/exp-command-buffer.yml b/unified-runtime/scripts/core/exp-command-buffer.yml
@@ -282,7 +282,7 @@ params:
       desc: "[in] Handle of the device object."
     - type: "const $x_exp_command_buffer_desc_t*"
       name: pCommandBufferDesc
-      desc: "[in][optional] command-buffer descriptor."
+      desc: "[in] Command-buffer descriptor."
     - type: "$x_exp_command_buffer_handle_t*"
       name: phCommandBuffer
       desc: "[out][alloc] Pointer to command-Buffer handle."
diff --git a/unified-runtime/source/adapters/cuda/command_buffer.cpp b/unified-runtime/source/adapters/cuda/command_buffer.cpp
@@ -352,10 +352,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext, ur_device_handle_t hDevice,
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     ur_exp_command_buffer_handle_t *phCommandBuffer) {
-
-  const bool IsUpdatable =
-      pCommandBufferDesc ? pCommandBufferDesc->isUpdatable : false;
-
+  const bool IsUpdatable = pCommandBufferDesc->isUpdatable;
   try {
     *phCommandBuffer =
         new ur_exp_command_buffer_handle_t_(hContext, hDevice, IsUpdatable);
diff --git a/unified-runtime/source/adapters/hip/command_buffer.cpp b/unified-runtime/source/adapters/hip/command_buffer.cpp
@@ -233,9 +233,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext, ur_device_handle_t hDevice,
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     ur_exp_command_buffer_handle_t *phCommandBuffer) {
-  const bool IsUpdatable =
-      pCommandBufferDesc ? pCommandBufferDesc->isUpdatable : false;
-
+  const bool IsUpdatable = pCommandBufferDesc->isUpdatable;
   try {
     *phCommandBuffer =
         new ur_exp_command_buffer_handle_t_(hContext, hDevice, IsUpdatable);
diff --git a/unified-runtime/source/adapters/level_zero/command_buffer.cpp b/unified-runtime/source/adapters/level_zero/command_buffer.cpp
@@ -586,9 +586,7 @@ bool canBeInOrder(ur_context_handle_t Context,
   bool CompatibleDriver = Context->getPlatform()->isDriverVersionNewerOrSimilar(
       1, 3, L0_DRIVER_INORDER_MIN_VERSION);
   bool CanUseDriverInOrderLists = CompatibleDriver && DriverInOrderRequested;
-  return CanUseDriverInOrderLists
-             ? (CommandBufferDesc ? CommandBufferDesc->isInOrder : false)
-             : false;
+  return CanUseDriverInOrderLists ? CommandBufferDesc->isInOrder : false;
 }
 
 /**
@@ -624,9 +622,8 @@ urCommandBufferCreateExp(ur_context_handle_t Context, ur_device_handle_t Device,
                          const ur_exp_command_buffer_desc_t *CommandBufferDesc,
                          ur_exp_command_buffer_handle_t *CommandBuffer) {
   bool IsInOrder = canBeInOrder(Context, CommandBufferDesc);
-  bool EnableProfiling =
-      CommandBufferDesc && CommandBufferDesc->enableProfiling && !IsInOrder;
-  bool IsUpdatable = CommandBufferDesc && CommandBufferDesc->isUpdatable;
+  bool EnableProfiling = CommandBufferDesc->enableProfiling && !IsInOrder;
+  bool IsUpdatable = CommandBufferDesc->isUpdatable;
   bool ImmediateAppendPath = checkImmediateAppendSupport(Context, Device);
   const bool WaitEventPath = !ImmediateAppendPath;
   bool UseCounterBasedEvents = checkCounterBasedEventsSupport(Device) &&
diff --git a/unified-runtime/source/adapters/mock/ur_mockddi.cpp b/unified-runtime/source/adapters/mock/ur_mockddi.cpp
@@ -8403,7 +8403,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext,
     /// [in] Handle of the device object.
     ur_device_handle_t hDevice,
-    /// [in][optional] command-buffer descriptor.
+    /// [in] Command-buffer descriptor.
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     /// [out][alloc] Pointer to command-Buffer handle.
     ur_exp_command_buffer_handle_t *phCommandBuffer) try {
diff --git a/unified-runtime/source/adapters/opencl/command_buffer.cpp b/unified-runtime/source/adapters/opencl/command_buffer.cpp
@@ -43,8 +43,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urCommandBufferCreateExp(
           CLContext, cl_ext::ExtFuncPtrCache->clCreateCommandBufferKHRCache,
           cl_ext::CreateCommandBufferName, &clCreateCommandBufferKHR));
 
-  const bool IsUpdatable =
-      pCommandBufferDesc ? pCommandBufferDesc->isUpdatable : false;
+  const bool IsUpdatable = pCommandBufferDesc->isUpdatable;
 
   ur_device_command_buffer_update_capability_flags_t UpdateCapabilities;
   cl_device_id CLDevice = cl_adapter::cast<cl_device_id>(hDevice);
diff --git a/unified-runtime/source/loader/layers/tracing/ur_trcddi.cpp b/unified-runtime/source/loader/layers/tracing/ur_trcddi.cpp
@@ -6978,7 +6978,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext,
     /// [in] Handle of the device object.
     ur_device_handle_t hDevice,
-    /// [in][optional] command-buffer descriptor.
+    /// [in] Command-buffer descriptor.
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     /// [out][alloc] Pointer to command-Buffer handle.
     ur_exp_command_buffer_handle_t *phCommandBuffer) {
diff --git a/unified-runtime/source/loader/layers/validation/ur_valddi.cpp b/unified-runtime/source/loader/layers/validation/ur_valddi.cpp
@@ -7615,7 +7615,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext,
     /// [in] Handle of the device object.
     ur_device_handle_t hDevice,
-    /// [in][optional] command-buffer descriptor.
+    /// [in] Command-buffer descriptor.
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     /// [out][alloc] Pointer to command-Buffer handle.
     ur_exp_command_buffer_handle_t *phCommandBuffer) {
@@ -7632,6 +7632,9 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferCreateExp(
     if (NULL == hDevice)
       return UR_RESULT_ERROR_INVALID_NULL_HANDLE;
 
+    if (NULL == pCommandBufferDesc)
+      return UR_RESULT_ERROR_INVALID_NULL_POINTER;
+
     if (NULL == phCommandBuffer)
       return UR_RESULT_ERROR_INVALID_NULL_POINTER;
   }
diff --git a/unified-runtime/source/loader/ur_ldrddi.cpp b/unified-runtime/source/loader/ur_ldrddi.cpp
@@ -7044,7 +7044,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext,
     /// [in] Handle of the device object.
     ur_device_handle_t hDevice,
-    /// [in][optional] command-buffer descriptor.
+    /// [in] Command-buffer descriptor.
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     /// [out][alloc] Pointer to command-Buffer handle.
     ur_exp_command_buffer_handle_t *phCommandBuffer) {
diff --git a/unified-runtime/source/loader/ur_libapi.cpp b/unified-runtime/source/loader/ur_libapi.cpp
@@ -7619,6 +7619,7 @@ ur_result_t UR_APICALL urBindlessImagesSignalExternalSemaphoreExp(
 ///         + `NULL == hContext`
 ///         + `NULL == hDevice`
 ///     - ::UR_RESULT_ERROR_INVALID_NULL_POINTER
+///         + `NULL == pCommandBufferDesc`
 ///         + `NULL == phCommandBuffer`
 ///     - ::UR_RESULT_ERROR_INVALID_CONTEXT
 ///     - ::UR_RESULT_ERROR_INVALID_DEVICE
@@ -7633,7 +7634,7 @@ ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext,
     /// [in] Handle of the device object.
     ur_device_handle_t hDevice,
-    /// [in][optional] command-buffer descriptor.
+    /// [in] Command-buffer descriptor.
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     /// [out][alloc] Pointer to command-Buffer handle.
     ur_exp_command_buffer_handle_t *phCommandBuffer) try {
diff --git a/unified-runtime/source/ur_api.cpp b/unified-runtime/source/ur_api.cpp
@@ -6664,6 +6664,7 @@ ur_result_t UR_APICALL urBindlessImagesSignalExternalSemaphoreExp(
 ///         + `NULL == hContext`
 ///         + `NULL == hDevice`
 ///     - ::UR_RESULT_ERROR_INVALID_NULL_POINTER
+///         + `NULL == pCommandBufferDesc`
 ///         + `NULL == phCommandBuffer`
 ///     - ::UR_RESULT_ERROR_INVALID_CONTEXT
 ///     - ::UR_RESULT_ERROR_INVALID_DEVICE
@@ -6678,7 +6679,7 @@ ur_result_t UR_APICALL urCommandBufferCreateExp(
     ur_context_handle_t hContext,
     /// [in] Handle of the device object.
     ur_device_handle_t hDevice,
-    /// [in][optional] command-buffer descriptor.
+    /// [in] Command-buffer descriptor.
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     /// [out][alloc] Pointer to command-Buffer handle.
     ur_exp_command_buffer_handle_t *phCommandBuffer) {
diff --git a/unified-runtime/test/conformance/exp_command_buffer/fixtures.h b/unified-runtime/test/conformance/exp_command_buffer/fixtures.h
@@ -60,8 +60,12 @@ struct urCommandBufferExpTest : uur::urContextTest {
     UUR_RETURN_ON_FATAL_FAILURE(uur::urContextTest::SetUp());
 
     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,
+    };
     ASSERT_SUCCESS(
-        urCommandBufferCreateExp(context, device, nullptr, &cmd_buf_handle));
+        urCommandBufferCreateExp(context, device, &desc, &cmd_buf_handle));
     ASSERT_NE(cmd_buf_handle, nullptr);
   }
 
@@ -83,8 +87,11 @@ struct urCommandBufferExpTestWithParam : urQueueTestWithParam<T> {
     UUR_RETURN_ON_FATAL_FAILURE(uur::urQueueTestWithParam<T>::SetUp());
 
     UUR_RETURN_ON_FATAL_FAILURE(checkCommandBufferSupport(this->device));
-    ASSERT_SUCCESS(urCommandBufferCreateExp(this->context, this->device,
-                                            nullptr, &cmd_buf_handle));
+
+    ur_exp_command_buffer_desc_t desc{UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC,
+                                      nullptr, false, false, false};
+    ASSERT_SUCCESS(urCommandBufferCreateExp(this->context, this->device, &desc,
+                                            &cmd_buf_handle));
     ASSERT_NE(cmd_buf_handle, nullptr);
   }
 
@@ -105,8 +112,11 @@ struct urCommandBufferExpExecutionTest : uur::urKernelExecutionTest {
     UUR_RETURN_ON_FATAL_FAILURE(uur::urKernelExecutionTest::SetUp());
 
     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};
     ASSERT_SUCCESS(
-        urCommandBufferCreateExp(context, device, nullptr, &cmd_buf_handle));
+        urCommandBufferCreateExp(context, device, &desc, &cmd_buf_handle));
     ASSERT_NE(cmd_buf_handle, nullptr);
   }
 
@@ -333,15 +343,8 @@ struct urCommandEventSyncTest : urCommandBufferExpTest {
       ASSERT_NE(buffer, nullptr);
     }
 
-    // Create a command-buffer with update enabled.
-    ur_exp_command_buffer_desc_t desc{
-        /*.stype=*/UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC,
-        /*.pNext =*/nullptr,
-        /*.isUpdatable =*/false,
-        /*.isInOrder =*/false,
-        /*.enableProfiling =*/false,
-    };
-
+    ur_exp_command_buffer_desc_t desc{UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC,
+                                      nullptr, true, false, false};
     ASSERT_SUCCESS(urCommandBufferCreateExp(context, device, &desc,
                                             &second_cmd_buf_handle));
     ASSERT_NE(second_cmd_buf_handle, nullptr);
diff --git a/unified-runtime/test/conformance/exp_command_buffer/kernel_event_sync.cpp b/unified-runtime/test/conformance/exp_command_buffer/kernel_event_sync.cpp
@@ -40,7 +40,10 @@ struct KernelCommandEventSyncTest
     ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 3, nullptr, device_ptrs[1]));
 
     // Create second command-buffer
-    ASSERT_SUCCESS(urCommandBufferCreateExp(context, device, nullptr,
+    ur_exp_command_buffer_desc_t desc{
+        UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC, nullptr, false, false, false,
+    };
+    ASSERT_SUCCESS(urCommandBufferCreateExp(context, device, &desc,
                                             &second_cmd_buf_handle));
     ASSERT_NE(second_cmd_buf_handle, nullptr);
   }
diff --git a/unified-runtime/test/conformance/exp_command_buffer/update/invalid_update.cpp b/unified-runtime/test/conformance/exp_command_buffer/update/invalid_update.cpp
@@ -109,8 +109,11 @@ TEST_P(InvalidUpdateTest, NotFinalizedCommandBuffer) {
 TEST_P(InvalidUpdateTest, NotUpdatableCommandBuffer) {
   // Create a command-buffer without isUpdatable
   ur_exp_command_buffer_handle_t test_cmd_buf_handle = nullptr;
+  ur_exp_command_buffer_desc_t desc{
+      UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC, nullptr, false, false, false,
+  };
   ASSERT_SUCCESS(
-      urCommandBufferCreateExp(context, device, nullptr, &test_cmd_buf_handle));
+      urCommandBufferCreateExp(context, device, &desc, &test_cmd_buf_handle));
   EXPECT_NE(test_cmd_buf_handle, nullptr);
 
   // Append a kernel commands to command-buffer and close command-buffer
diff --git a/unified-runtime/test/conformance/program/urMultiDeviceProgramCreateWithBinary.cpp b/unified-runtime/test/conformance/program/urMultiDeviceProgramCreateWithBinary.cpp
@@ -311,9 +311,12 @@ TEST_P(urMultiDeviceCommandBufferExpTest, Enqueue) {
     }
 
     // Create command-buffer
+    ur_exp_command_buffer_desc_t desc{
+        UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_DESC, nullptr, false, false, false,
+    };
     uur::raii::CommandBuffer cmd_buf_handle;
-    ASSERT_SUCCESS(urCommandBufferCreateExp(context, device, nullptr,
-                                            cmd_buf_handle.ptr()));
+    ASSERT_SUCCESS(
+        urCommandBufferCreateExp(context, device, &desc, cmd_buf_handle.ptr()));
 
     // Append kernel command to command-buffer and close command-buffer
     ASSERT_SUCCESS(urCommandBufferAppendKernelLaunchExp(
