OpenCL-Headers icon indicating copy to clipboard operation
OpenCL-Headers copied to clipboard

Breaking change rename of `cl_ndrange_kernel_command_properties_khr` to `cl_command_properties_khr`

Open amaiorano opened this issue 1 year ago • 3 comments

The recent headers update includes a rename of cl_ndrange_kernel_command_properties_khr to cl_command_properties_khr. This broke a bunch of targets when importing this repo into Google's repos, including TensorFlow.

Please add a using alias of cl_ndrange_kernel_command_properties_khr to cl_command_properties_khr? You can aslo add a [[deprecated]] attribute to the using alias so that a warning shows up, allowing clients to update their code.

amaiorano avatar Oct 18 '24 14:10 amaiorano

Pinging @bashbaug at David Neto's request

amaiorano avatar Oct 18 '24 14:10 amaiorano

Please add a using alias of cl_ndrange_kernel_command_properties_khr to cl_command_properties_khr? You can aslo add a [[deprecated]] attribute to the using alias so that a warning shows up, allowing clients to update their code.

It's a C header so keeping the old typedef should work?

dneto0 avatar Oct 18 '24 14:10 dneto0

As discussed during the October 23rd teleconference, this is the intended behaviour. cl_khr_command_buffer is a provisional extension, thus the OpenCL workgroup does not guarantee that breaking changes won't happen.

The group vision is to enforce that using an opt-in mechanism so that developers are aware of what they expose themselves to https://github.com/KhronosGroup/OpenCL-Headers/issues/247.

Of course, this could be fixed by keeping cl_ndrange_kernel_command_properties_khr as an alias to cl_command_properties_khr, but in the end, I think we would be better off just fixing it in tensorflow.

Here are a Google internal patches for it:

  • https://critique.corp.google.com/cl/688858221
  • https://critique.corp.google.com/cl/688859476

Which look like that:

--- a/lite/delegates/gpu/cl/opencl_wrapper.h
+++ b/lite/delegates/gpu/cl/opencl_wrapper.h
+ #if CL_KHR_COMMAND_BUFFER_EXTENSION_VERSION >= CL_MAKE_VERSION(0, 9, 5)
+ typedef cl_int(CL_API_CALL *PFN_clCommandNDRangeKernelKHR)(
+     cl_command_buffer_khr /*command_buffer*/,
+     cl_command_queue /*command_queue*/,
+     const cl_command_properties_khr * /*properties*/,
+     cl_kernel /*kernel*/, cl_uint /*work_dim*/,
+     const size_t * /*global_work_offset*/, const size_t * /*global_work_size*/,
+     const size_t * /*local_work_size*/,
+     cl_uint /*num_sync_points_in_wait_list*/,
+     const cl_sync_point_khr * /*sync_point_wait_list*/,
+     cl_sync_point_khr * /*sync_point*/,
+     cl_mutable_command_khr * /*mutable_handle*/);
+ #else
  typedef cl_int(CL_API_CALL *PFN_clCommandNDRangeKernelKHR)(
      cl_command_buffer_khr /*command_buffer*/,
      cl_command_queue /*command_queue*/,
      const cl_ndrange_kernel_command_properties_khr * /*properties*/,
      cl_kernel /*kernel*/, cl_uint /*work_dim*/,
      const size_t * /*global_work_offset*/, const size_t * /*global_work_size*/,
      const size_t * /*local_work_size*/,
      cl_uint /*num_sync_points_in_wait_list*/,
      const cl_sync_point_khr * /*sync_point_wait_list*/,
      cl_sync_point_khr * /*sync_point*/,
      cl_mutable_command_khr * /*mutable_handle*/);
+ #endif

rjodinchr avatar Oct 23 '24 08:10 rjodinchr