Implement cl_khr_command_buffer_multi_device #1437

jansol · 2024-03-11T22:37:16Z

With command buffers being implemented "host-side", this is a fairly straightforward thing to support.

jansol · 2024-03-11T22:38:10Z

The formatting scripts had gotten out of sync with each other recently so this also brings those back in line.

pjaaskel

Generally looks good, but can you run the formatter (at least long lines pop to eye) and add a bit of comments to the overall functioning of the current remap()? Also perhaps squash some of the commits together if it's not too much trouble (I accidentally commented some obsolete issues related to multiple devices, I think, when going through the commits).

pjaaskel · 2024-03-12T12:49:43Z

lib/CL/clGetDeviceInfo.c

+
+  case CL_DEVICE_COMMAND_BUFFER_NUM_SYNC_DEVICES_KHR:
+    if (device->ops->get_device_info_ext != NULL
+        && device->ops->get_device_info_ext(device, param_name, param_value_size,param_value, param_value_size_ret) == CL_SUCCESS)


This line looks long. Did you run the formatter?

pjaaskel · 2024-03-12T12:50:42Z

lib/CL/clRemapCommandBufferKHR.c

@@ -0,0 +1,124 @@
+/* OpenCL runtime library: clRemapCommandBufferKHR()
+
+   Copyright (c) 2022 Jan Solanti / Tampere University


Was this really done in 2022?

That's when the initial (private) commits are from. I'll update it to a year range.

pjaaskel · 2024-03-12T12:51:02Z

lib/CL/clRemapCommandBufferKHR.c

+
+
+#define CMD_NODE_COPY_ARRAY(item_t, count, name) \
+  do { \


The indent of this block is wrong.

pjaaskel · 2024-03-12T12:53:31Z

lib/CL/clRemapCommandBufferKHR.c

+  POCL_GOTO_ERROR_COND ((new_cmd->name == NULL), CL_OUT_OF_HOST_MEMORY); \
+  memcpy(new_cmd->name, cmd->name, sizeof (item_t) * count); \
+  }while(0)
+


A comment documenting the overall functionality of the current remap would be useful here.

pjaaskel · 2024-03-12T12:54:52Z

lib/CL/pocl_util.c

@@ -1228,6 +1228,18 @@ pocl_cmdbuf_choose_recording_queue (cl_command_buffer_khr command_buffer,
  return CL_SUCCESS;
 }

+cl_command_buffer_properties_khr
+pocl_cmdbuf_get_property (cl_command_buffer_khr command_buffer,
+              cl_command_buffer_properties_khr name)


pjaaskel · 2024-03-12T12:58:53Z

tests/runtime/test_command_buffer_multi_device.c

+      for (unsigned i = 0; i < extensions_size / sizeof(cl_name_version); ++i)
+      {
+        cl_name_version *e = extensions+i;
+        if (strcmp(e->name, "cl_khr_command_buffer_multi_device") == 0)


Format doesn't look correct.

pjaaskel · 2024-03-12T13:06:58Z

lib/CL/clRemapCommandBufferKHR.c

@@ -59,23 +59,29 @@ POname (clRemapCommandBufferKHR) (
  cl_command_buffer_properties_khr universal_sync = pocl_cmdbuf_get_property(command_buffer, CL_COMMAND_BUFFER_UNIVERSAL_SYNC_KHR);
  POCL_GOTO_ERROR_COND((universal_sync == 0 && num_queues > 1), CL_INCOMPATIBLE_COMMAND_QUEUE_KHR);

-  POname (clCreateCommandBufferKHR) (num_queues, queues, command_buffer->properties, &errcode);
+  new_cmdbuf = POname (clCreateCommandBufferKHR) (num_queues, queues, command_buffer->properties, &errcode);


Format? Long line.

pjaaskel · 2024-03-12T13:09:58Z

tests/runtime/test_command_buffer_multi_device.c

@@ -303,7 +303,7 @@ main (int _argc, char **_argv)
      CHECK_CL_ERROR (clReleaseEvent (command_buf_event));
    }

-  /* Remap from 2 to 1 queues & run */
+  /* Remap from N to 1 queues & run */


Can we test the opposite case as well? That is, the one that can enable runtime-driven task mapping/perf. portability via pushing all work to a default CPU queue and then remapping to utilize more devices. That likely is a NOP for now. Also it would be good to test a case where the remap is done to queue/devices which are not in the original schedule/buffer. This models the disappearing devices case.

The current automatic remapping logic is very simple and can't make use of a larger queue list (and is very likely to degrade performance if the number of devices changes.) This is definitely something that should be added in the future, though.

pjaaskel · 2024-03-12T13:12:25Z

lib/CL/clCreateCommandBufferKHR.c

@@ -96,6 +104,8 @@ POname (clCreateCommandBufferKHR) (
        }
    }

+  POCL_GOTO_ERROR_COND ((num_queues > 1 && !pocl_cmdbuf_can_queues_sync(num_queues, queues, universal_sync_enabled)), CL_INCOMPATIBLE_COMMAND_QUEUE_KHR);


Long lines here as well.

pjaaskel · 2024-03-12T13:12:56Z

lib/CL/pocl_util.c

+          cl_device_id dev = queues[i]->device;
+
+          cl_uint num_sync_devices;
+          POCL_RETURN_ERROR_COND ((POname(clGetDeviceInfo)(queues[i]->device, CL_DEVICE_COMMAND_BUFFER_NUM_SYNC_DEVICES_KHR, sizeof (cl_uint), &num_sync_devices, NULL)), CL_INCOMPATIBLE_COMMAND_QUEUE_KHR);


pjaaskel · 2024-03-28T07:12:24Z

Can you describe this in the release notes and rebase?

jansol · 2024-04-04T15:31:40Z

Rebased, squashed & formatted. Should be easier to review now. I'm seeing a number of asan warnings though (locally as well), still working through those.

pjaaskel · 2024-04-08T09:31:49Z

Can you describe this in the release notes and rebase?

@jansol: a release note is still missing.

pjaaskel · 2024-04-09T08:55:20Z

doc/sphinx/source/notes_6_0.rst

+Multi-device command buffers
+============================
+
+Initial support for `cl_khr_command_buffer_multi_device` has been added, i.e. it


For the CPU devices, right?

There is not really anything driver-specific, just like in the original command buffer code. It just hasn't really been tested with other drivers.

Perhaps still worth mentioning what are the expectations in terms of perf. improvements (none still, right?) as it "caches" mainly the error checking?

cmdbuf: handle profiling queries for multidev buffers cmdbuf: add new platform & device queries multidev command buffers & remap clRemapCommandBufferKHR: leave new command buffer in same state as the original advertise cl_khr_command_buffer_multi_device clEnqueueCommandBufferKHR: fix helper function name command buffers: add multi device test cmdbuf: make clRemapCommandBufferKHR actually available cmdbuf: various bugfixes cmdbuf: more correct cmdbuf flag handling tests/cmdbuf: add automatic remapping flag to clRemapCommandBufferKHR lib: update command buffer code to conform to latest specs Rework *ndrangekernel & cmdbuf remapping multidev-cmdbuf: Use correct queues to run cmds tests: fix memory leaks in multidev cmdbuf test tests: add missing include

ASAN was making a lot of noise about these

pjaaskel · 2024-04-18T08:44:17Z

The arm bots are broken again, pulling this in regardless.

pjaaskel requested changes Mar 12, 2024

View reviewed changes

jansol force-pushed the multidev_cmdbuf branch 3 times, most recently from ff712de to 59016cb Compare April 4, 2024 15:11

jansol force-pushed the multidev_cmdbuf branch from c041ed0 to 72cb2e7 Compare April 5, 2024 16:34

jansol force-pushed the multidev_cmdbuf branch from 72cb2e7 to 70cc91e Compare April 8, 2024 18:47

pjaaskel reviewed Apr 9, 2024

View reviewed changes

jansol force-pushed the multidev_cmdbuf branch 2 times, most recently from 3c79fc1 to c19ce9a Compare April 12, 2024 15:03

jansol added 8 commits April 12, 2024 18:34

tools: fix & unify formatting scripts again

c9c9308

ignore .gdb_history

7325670

ci: update RDMA to LLVM 17

c0bfdc4

cmdbuf: various small fixes

a6a5ade

cmdbuf: move queue list validation to its own function

8b520a6

Fix some memoryleaks in tests

4dbb7d0

ASAN was making a lot of noise about these

multidev cmd buffers: advertise in release notes

e450b7e

jansol force-pushed the multidev_cmdbuf branch from c19ce9a to e450b7e Compare April 12, 2024 15:40

pjaaskel merged commit 293add4 into pocl:main Apr 18, 2024
34 checks passed

jansol deleted the multidev_cmdbuf branch April 18, 2024 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cl_khr_command_buffer_multi_device #1437

Implement cl_khr_command_buffer_multi_device #1437

jansol commented Mar 11, 2024

jansol commented Mar 11, 2024

pjaaskel left a comment

pjaaskel Mar 12, 2024

pjaaskel Mar 12, 2024

jansol Mar 12, 2024

pjaaskel Mar 12, 2024

pjaaskel Mar 12, 2024

pjaaskel Mar 12, 2024

pjaaskel Mar 12, 2024

pjaaskel Mar 12, 2024

pjaaskel Mar 12, 2024

jansol Mar 12, 2024

pjaaskel Mar 12, 2024

pjaaskel Mar 12, 2024

pjaaskel commented Mar 28, 2024

jansol commented Apr 4, 2024

pjaaskel commented Apr 8, 2024

pjaaskel Apr 9, 2024

jansol Apr 9, 2024

pjaaskel Apr 9, 2024

pjaaskel commented Apr 18, 2024

		@@ -0,0 +1,124 @@
		/* OpenCL runtime library: clRemapCommandBufferKHR()

		Copyright (c) 2022 Jan Solanti / Tampere University

Implement cl_khr_command_buffer_multi_device #1437

Implement cl_khr_command_buffer_multi_device #1437

Conversation

jansol commented Mar 11, 2024

jansol commented Mar 11, 2024

pjaaskel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjaaskel commented Mar 28, 2024

jansol commented Apr 4, 2024

pjaaskel commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjaaskel commented Apr 18, 2024