Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking change rename of cl_ndrange_kernel_command_properties_khr to cl_command_properties_khr #266

Open
amaiorano opened this issue Oct 18, 2024 · 3 comments

Comments

@amaiorano
Copy link

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
Copy link
Author

Pinging @bashbaug at David Neto's request

@dneto0
Copy link

dneto0 commented Oct 18, 2024

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?

@rjodinchr
Copy link

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 #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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants