New cl_khr_command_buffer_mutable_memory_commands extension

[EwanC]

Discussion issue for the functionality to be able to update the parameters to memory operation commands in a command-buffer after the command-buffer has been finalized using the clUpdateMutableCommandsKHR entry-point defined by cl_khr_command_buffer_mutable_dispatch. Defined in a new layered extension - cl_khr_command_buffer_mutable_memory_commands

The initial API draft to implement this functionality used a find/replaced based lookup for cl_mem objects used as command parameters. However this was not determined to be a robust solution as 1) src/dest could potentially be the same buffer for commands like clCommandCopyBufferRectKHR and 2) can't change non cl_mem parameters to commands like size

An alternative draft was therefore designed which has a specific struct for each command that gives configurations per command which solves these issues. See #1065 for the PR containing the extension draft using this mechanism.

A drawback of this design, other than being more verbose, is that is requires the users to keep around all of the original information about the command. They can't just put in a new cl_mem if that's the only aspect of a command they want to update, they need to put in the new cl_mem and then also reinsert the original information about the command.

To get around this drawback it was proposed we could have reusable structures per command. e.g. A struct for updating the source cl_mem, as struct update the destination cl_mem, a struct for updating both the source and destination cl_mem, etc. Whether we want to go forward with this API proposal is where the discussion should be continued from.

[EwanC]

To get around this drawback it was proposed we could have reusable structures per command. e.g. A struct for updating the source cl_mem, as struct update the destination cl_mem, a struct for updating both the source and destination cl_mem, etc. Whether we want to go forward with this API proposal is where the discussion should be continued from.

As an exercise I've tried to draft out how many structs we could possibly have if we used the parameter being updated as the primary type in reusable structs for all the different command parameters. For an initial release we'd probably only want a subset of these though, maybe cl_mutable_mem_command_src_mem_khr,cl_mutable_mem_command_dst_mem_khr, cl_mutable_mem_command_src_pointer_khr, cl_mutable_mem_command_dst_pointer_khr , cl_mutable_mem_command_size_khr which would let you change all the aspects of buffer/SVM mem copies as the buffer offset could be worked around with sub-buffers.

// clCommandCopyBufferKHR, clCommandCopyBufferRectKHR,
// clCommandCopyBufferToImageKHR, clCommandCopyImageKHR,
// clCommandCopyImageToBufferKHR, clCommandFillBufferKHR,
// clCommandFillImageKHR
struct cl_mutable_mem_command_src_mem_khr {
  cl_mem                        src_buffer;
  cl_uint                       num_mem_commands;
  const cl_mutable_command_khr* mem_command_list;
};

// clCommandCopyBufferKHR, clCommandCopyBufferRectKHR,
// clCommandCopyBufferToImageKHR, clCommandCopyImageKHR,
// clCommandCopyImageToBufferKHR
struct cl_mutable_mem_command_dst_mem_khr {
  cl_mem                        dst_buffer;
  cl_uint                       num_mem_commands;
  const cl_mutable_command_khr* mem_command_list;
};

// clCommandSVMMemcpyKHR, clCommandSVMMemFillKHR
struct cl_mutable_mem_command_src_pointer_khr {
  void*                         src_ptr;
  cl_uint                       num_mem_commands;
  const cl_mutable_command_khr* mem_command_list;
};

// clCommandSVMMemcpyKHR
struct cl_mutable_mem_command_dst_pointer_khr {
  void*                         dst_ptr;
  cl_uint                       num_mem_commands;
  const cl_mutable_command_khr* mem_command_list;
};

// clCommandCopyBufferKHR, clCommandFillBufferKHR,
// clCommandSVMMemcpyKHR, clCommandSVMMemFillKHR
struct cl_mutable_mem_command_size_khr {
  size_t                        size;
  cl_uint                       num_mem_commands;
  const cl_mutable_command_khr* mem_command_list;
};

// clCommandSVMMemFillKHR, clCommandFillBufferKHR
struct cl_mutable_mem_command_pattern_khr {
  const void*                   pattern;
  size_t                        pattern_size;
  cl_uint                       num_mem_commands;
  const cl_mutable_command_khr* mem_command_list;
};

// clCommandCopyBufferKHR, clCommandCopyBufferToImageKHR,
// clCommandFillBufferKHR
struct cl_mutable_mem_command_src_offset_khr {
  size_t                         src_offset;
  cl_uint                        num_mem_commands;
  const cl_mutable_command_khr*  mem_command_list;
};

// clCommandCopyBufferKHR, clCommandCopyImageToBufferKHR
struct cl_mutable_mem_command_dst_offset_khr {
  size_t                         dst_offset;
  cl_uint                        num_mem_commands;
  const cl_mutable_command_khr*  mem_command_list;
};

// clCommandCopyBufferRectKHR, clCommandFillImageKHR
// clCommandCopyImageToBufferKHR, clCommandCopyImageKHR
struct cl_mutable_mem_command_src_origin_khr {
  const size_t*                  src_origin;
  cl_uint                        num_mem_commands;
  const cl_mutable_command_khr*  mem_command_list;
};

// clCommandCopyBufferRectKHR,
// clCommandCopyBufferToImageKHR, clCommandCopyImageKHR
struct cl_mutable_mem_command_dst_origin_khr {
  const size_t*                  dst_origin;
  cl_uint                        num_mem_commands;
  const cl_mutable_command_khr*  mem_command_list;
};

// clCommandCopyBufferRectKHR, clCommandCopyImageToBufferKHR
// clCommandCopyBufferToImageKHR, clCommandCopyImageKHR,
// clCommandFillImageKHR
struct cl_mutable_mem_command_region_khr {
  const size_t*                  region;
  cl_uint                        num_mem_commands;
  const cl_mutable_command_khr*  mem_command_list;
};

// clCommandCopyBufferRectKHR
struct cl_mutable_mem_command_copy_rect_khr {
  size_t                         src_row_pitch;
  size_t                         src_slice_pitch;
  size_t                         dst_row_pitch;
  size_t                         dst_slice_pitch;
  cl_uint                        num_mem_commands;
  const cl_mutable_command_khr*  mem_command_list;
};

[EwanC]

There was a question on the 19/03/24 WG teleconference about whether we could use the current drafted approach of structs matching the API parameters, and use some special value to signify no update for members we don't want to update (to avoid the user having to remember the old values).

Thinking about this just for size parameters, issue #1072 clarifies that 0 is a valid size value (from OpenCL 2.1 anyway) . As a result I'm not sure what value we could use as a special value that wouldn't eat into the range of valid values. Especially as it is a size_t typed parameter, what is the top end of usable size_t values on 32/64 bit may not be a good choice for the other arch.

[EwanC]

There was a question on the 19/03/24 WG teleconference about whether we could use the current drafted approach of structs matching the API parameters, and use some special value to signify no update for members we don't want to update (to avoid the user having to remember the old values).

Mulling this over some more, we could make all the members of the structs pointers where NULL would signify no update.

So to take clCommandCopyBufferKHR as an example, instead of what we have now:

typedef struct cl_mutable_copy_buffer_command_config_khr {
    cl_mutable_command_khr command;
    cl_mem src_buffer;
    cl_mem dst_buffer;
    size_t src_offset;
    size_t dst_offset;
    size_t size;
} cl_mutable_copy_buffer_command_config_khr;

We'd make this

typedef struct cl_mutable_copy_buffer_command_config_khr {
    cl_mutable_command_khr command;
    cl_mem* src_buffer;
    cl_mem* dst_buffer;
    size_t* src_offset;
    size_t* dst_offset;
    size_t* size;
} cl_mutable_copy_buffer_command_config_khr;

Then a user could choose to set as many members as they wanted, and not be forced to remember the old values for the unchanged members. For example, to only change the src & dst buffers but not offset or size:

 cl_mutable_copy_buffer_command_config_khr buffer_copy_config = {
      copy_command_handle,  // command
      &new_src_buffer,  // src_buffer
      &new_dst_buffer, // dst_buffer
      nullptr, // src_offset
      nullptr,   // dst_offset
      nullptr   // size
  };

[SunSerega]

I think this would make these structures awkward to use in both native code like C/C++ (e.g. when the user wants to declare an inline array with some simple literals for values) and high-level code like C# (avoiding exposing these pointers in the interface breeds hacks or a lot of extra copy operations, AFAICT).

And, well, using pointers with their special NULL value here feels like a hack. The std::optional pattern generally only needs an extra bool field.
Adding this field might feel like too simplistic of a solution, but I think it's at least better than using pointers here.

P.S. Or, another option - how about a bitfield, with bits set corresponding to the values the user wants to update?

[EwanC]

That's good feedback thanks 🙂 I was thinking that the less struct members less intimidating to users the interface would be, but being able to declare the struct members inline is probably the ideal usage.

For the std::optional like pattern, it might be hard to replicate a container like that in C without templates unless we want to enumerate all the types that could be updated. So i'm not sure if the bool member would just come before the member to update?

typedef struct cl_mutable_copy_buffer_command_config_khr {
    cl_mutable_command_khr command;
    bool update_src_buffer;
    cl_mem src_buffer;
    bool update_dst_buffer;
    cl_mem dst_buffer;
    bool update_src_offset;
    size_t src_offset;
    bool update_dst_offset;
    size_t dst_offset;
    bool update_size;
    size_t size;
} cl_mutable_copy_buffer_command_config_khr;

The bitfield solution would be less verbose across the whole struct, but concentrate the complexity in a single member, which I maybe have a slight preference for but need to mull it over some more. Thanks again for the suggestions.

[SunSerega]

it might be hard to replicate a container like that in C

Yeah, I didn't mean declaring another type for it, I only meant adding extra bool fields as you did in the code above, to simulate the same pattern.

But for the bitfield solution, we can make an extra enum similar to CL_DEVICE_TYPE_ALL, containing all the bits, to remove complexity from the most generic case.
In that case, the user will only need to add complexity when doing partial updates, by including/excluding specific bits.
The main minus I see is that each struct will need its own enum group, and they will probably have lengthy names...