[UR][Graph] Strengthen in-order command-buffer property

Actions issue #18330 to
enhance the semantics of the UR command-buffer in-order property
such that sync-point dependencies will always be ignored.

Author: EwanC

unified-runtime/include/ur_api.h

@@ -59,8 +59,8 @@ constructed. The members defined in ${x}_exp_command_buffer_desc_t are:
 
 * ``isUpdatable``, which should be set to ``true`` to support :ref:`updating
   command-buffer commands`.
-* ``isInOrder``, which should be set to ``true`` to enable commands enqueued to
-  a command-buffer to be executed in an in-order fashion where possible.
+* ``isInOrder``, which should be set to ``true`` to enforce commands appended
+  to a command-buffer to be executed in an in-order fashion.
 * ``enableProfiling``, which should be set to ``true`` to enable profiling of
   the command-buffer.
 
@@ -108,8 +108,9 @@ Sync-Points
 A sync-point is a value which represents a command inside of a command-buffer
 which is returned from command-buffer append function calls. These can be
 optionally passed to these functions to define execution dependencies on other
-commands within the command-buffer. Sync-points passed to functions may be
-ignored if the command-buffer was created in-order.
+commands within the command-buffer. Both wait-list and return sync-point
+parameters to append functions are ignored if the command-buffer was created
+with the in-order property.
 
 Sync-points are unique and valid for use only within the command-buffer they
 were obtained from.
@@ -550,6 +551,9 @@ Changelog
 +-----------+-------------------------------------------------------+
 | 1.11      | Support native commands.                              |
 +-----------+-------------------------------------------------------+
+| 1.12      | Strengthen in-order property such that sync-points    |
+|           | parameters to append APIs are ignored.                |
++-----------+-------------------------------------------------------+
 
 Contributors
 --------------------------------------------------------------------------------

unified-runtime/scripts/core/EXP-COMMAND-BUFFER.rst

@@ -143,7 +143,7 @@ members:
       desc: "[in] Commands in a finalized command-buffer can be updated."
     - type: $x_bool_t
       name: isInOrder
-      desc: "[in] Commands in a command-buffer may be executed in-order without explicit dependencies."
+      desc: "[in] Commands in a command-buffer will execute in-order, explicit sync-point dependencies are ignored."
     - type: $x_bool_t
       name: enableProfiling
       desc: "[in] Command-buffer profiling is enabled."
@@ -375,7 +375,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -440,7 +440,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -502,7 +502,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -569,7 +569,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -628,7 +628,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -687,7 +687,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -761,7 +761,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -835,7 +835,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -909,7 +909,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -971,7 +971,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -1032,7 +1032,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -1095,7 +1095,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: uint32_t
       name: numEventsInWaitList
       desc: "[in] Size of the event wait list."
@@ -1166,7 +1166,7 @@ params:
     - type: "const $x_exp_command_buffer_sync_point_t*"
       name: pSyncPointWaitList
       desc: "[in][optional] A list of sync points that this command depends on.
-      May be ignored if command-buffer is in-order."
+      Will be ignored if command-buffer is in-order."
     - type: "$x_exp_command_buffer_sync_point_t*"
       name: pSyncPoint
       desc: "[out][optional] Sync point associated with this command."

unified-runtime/scripts/core/exp-command-buffer.yml

@@ -56,10 +56,11 @@ ur_result_t commandHandleDestroy(
 } // end anonymous namespace
 
 ur_exp_command_buffer_handle_t_::ur_exp_command_buffer_handle_t_(
-    ur_context_handle_t Context, ur_device_handle_t Device, bool IsUpdatable)
+    ur_context_handle_t Context, ur_device_handle_t Device, bool IsUpdatable,
+    bool IsInOrder)
     : Context(Context), Device(Device), IsUpdatable(IsUpdatable),
-      CudaGraph{nullptr}, CudaGraphExec{nullptr}, RefCount{1},
-      NextSyncPoint{0} {
+      IsInOrder(IsInOrder), CudaGraph{nullptr}, CudaGraphExec{nullptr},
+      RefCount{1}, NextSyncPoint{0} {
   urContextRetain(Context);
 }
 
@@ -151,11 +152,22 @@ static ur_result_t getNodesFromSyncPoints(
   // the event associated with each sync-point
   auto SyncPoints = CommandBuffer->SyncPoints;
 
+  // If command-buffer is in-order use last node in ordered map
+  if (CommandBuffer->IsInOrder && !SyncPoints.empty()) {
+    auto LastNode = std::prev(SyncPoints.end());
+    CuNodesList.push_back(LastNode->second);
+  }
+
   // For each sync-point add associated CUDA graph node to the return list.
   for (size_t i = 0; i < NumSyncPointsInWaitList; i++) {
     if (auto NodeHandle = SyncPoints.find(SyncPointWaitList[i]);
         NodeHandle != SyncPoints.end()) {
-      CuNodesList.push_back(NodeHandle->second);
+      auto DepNode = NodeHandle->second;
+      // Cuda driver API won't let you add duplicates to the dependency list
+      if (std::find(CuNodesList.begin(), CuNodesList.end(), DepNode) ==
+          CuNodesList.end()) {
+        CuNodesList.push_back(DepNode);
+      }
     } else {
       return UR_RESULT_ERROR_INVALID_VALUE;
     }
@@ -346,9 +358,10 @@ UR_APIEXPORT ur_result_t UR_APICALL urCommandBufferCreateExp(
     const ur_exp_command_buffer_desc_t *pCommandBufferDesc,
     ur_exp_command_buffer_handle_t *phCommandBuffer) {
   const bool IsUpdatable = pCommandBufferDesc->isUpdatable;
+  const bool IsInOrder = pCommandBufferDesc->isInOrder;
   try {
-    *phCommandBuffer =
-        new ur_exp_command_buffer_handle_t_(hContext, hDevice, IsUpdatable);
+    *phCommandBuffer = new ur_exp_command_buffer_handle_t_(
+        hContext, hDevice, IsUpdatable, IsInOrder);
   } catch (const std::bad_alloc &) {
     return UR_RESULT_ERROR_OUT_OF_HOST_MEMORY;
   } catch (...) {

unified-runtime/source/adapters/cuda/command_buffer.cpp

@@ -131,7 +131,8 @@ struct ur_exp_command_buffer_command_handle_t_ : ur::cuda::handle_base {
 struct ur_exp_command_buffer_handle_t_ {
 
   ur_exp_command_buffer_handle_t_(ur_context_handle_t Context,
-                                  ur_device_handle_t Device, bool IsUpdatable);
+                                  ur_device_handle_t Device, bool IsUpdatable,
+                                  bool IsInOrder);
 
   ~ur_exp_command_buffer_handle_t_();
 
@@ -182,6 +183,8 @@ struct ur_exp_command_buffer_handle_t_ {
   ur_device_handle_t Device;
   // Whether commands in the command-buffer can be updated
   bool IsUpdatable;
+  // Whether commands in the command-buffer are in-order.
+  bool IsInOrder;
   // Cuda Graph handle
   CUgraph CudaGraph;
   // Cuda Graph Exec handle
@@ -190,9 +193,9 @@ struct ur_exp_command_buffer_handle_t_ {
   // using std::atomic prevents data race when incrementing/decrementing.
   std::atomic_uint32_t RefCount;
 
-  // Map of sync_points to ur_events
-  std::unordered_map<ur_exp_command_buffer_sync_point_t, CUgraphNode>
-      SyncPoints;
+  // Ordered map of sync_points to ur_events, so that we can find the last
+  // node added to an in-order command-buffer.
+  std::map<ur_exp_command_buffer_sync_point_t, CUgraphNode> SyncPoints;
   // Next sync_point value (may need to consider ways to reuse values if 32-bits
   // is not enough)
   ur_exp_command_buffer_sync_point_t NextSyncPoint;