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.

Sign up for GitHub

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

Jump to bottom

Update clUpdateMutableCommandsKHR to match new API #501

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update clUpdateMutableCommandsKHR to match new API #501

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a91f218
Use array for clUpdateMutableCommandsKHR
EwanC Jan 17, 2024
33ad6a6
Set correct extension version
EwanC Sep 5, 2024
e4f9f80
Clang-tidy fixes
EwanC Sep 23, 2024
84be0bf
const more mutable-dispatch test variables
EwanC Sep 24, 2024
abac7ad
Remove reinterpret_cast
EwanC Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions doc/source/cl/extension.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,20 +317,20 @@ before the extension can be turned on by default in the oneAPI Construction Kit:
* Event profiling is not implemented (see CA-3322).

.. _cl_khr_command_buffer:
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#cl_khr_command_buffer
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_API.html#cl_khr_command_buffer

Command Buffers: Mutable Dispatch - ``cl_khr_command_buffer_mutable_dispatch``
------------------------------------------------------------------------------

oneAPI Construction Kit implements version 0.9.0 of the provisional
oneAPI Construction Kit implements version 0.9.2 of the provisional
`cl_khr_command_buffer_mutable_dispatch`_ extension.

`cl_khr_command_buffer_mutable_dispatch`_ builds upon `cl_khr_command_buffer`_
to allow users to modify kernel execution commands between enqueues of a
command-buffer.

.. _cl_khr_command_buffer_mutable_dispatch:
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#cl_khr_command_buffer_mutable_dispatch
https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_API.html#cl_khr_command_buffer_mutable_dispatch

Extended Async Copies - ``cl_khr_extended_async_copies``
---------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion external/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ set(CMAKE_CXX_EXTENSIONS ON)
FetchContent_Declare(
OpenCLHeaders
GIT_REPOSITORY https://github.com/KhronosGroup/OpenCL-Headers.git
GIT_TAG v2024.05.08
GIT_TAG 7258b9e0137474c988142fd35777b59dcdff167e
)
FetchContent_MakeAvailable(OpenCLHeaders)

Expand Down
16 changes: 8 additions & 8 deletions source/cl/examples/MutableDispatchKHR/source/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,8 @@ int main(const int argc, const char **argv) {
&input_B_buffer};
const cl_mutable_dispatch_arg_khr arg_2{2, sizeof(cl_mem),
&output_buffer};
const cl_mutable_dispatch_arg_khr args[] = {arg_0, arg_1, arg_2};
const cl_mutable_dispatch_config_khr dispatch_config{
CL_STRUCTURE_TYPE_MUTABLE_DISPATCH_CONFIG_KHR,
nullptr,
cl_mutable_dispatch_arg_khr args[] = {arg_0, arg_1, arg_2};
cl_mutable_dispatch_config_khr dispatch_config{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the clang-tidy reports on the earlier version for variables that could be const, I am surprised that none are raised here. Could you check whether this really needs to be non-const, or whether the source file was skipped in the check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy was being run (I manually run the tidy-clMutableDispathcKHR target to check) but the tool didn't seem to mandate const on these, but I went through and added const anyway

command_handle,
3 /* num_args */,
0 /* num_svm_arg */,
Expand All @@ -230,12 +228,14 @@ int main(const int argc, const char **argv) {
nullptr /* global_work_offset */,
nullptr /* global_work_size */,
nullptr /* local_work_size */};
const cl_mutable_base_config_khr mutable_config{
CL_STRUCTURE_TYPE_MUTABLE_BASE_CONFIG_KHR, nullptr, 1,
&dispatch_config};

// Update the command buffer with the mutable configuration
error = clUpdateMutableCommandsKHR(command_buffer, &mutable_config);
const cl_uint num_configs = 1;
cl_command_buffer_update_type_khr config_types[1] = {
CL_STRUCTURE_TYPE_MUTABLE_DISPATCH_CONFIG_KHR};
const void *configs[1] = {&dispatch_config};
error = clUpdateMutableCommandsKHR(command_buffer, num_configs,
config_types, configs);
CL_CHECK(error);
}

Expand Down
4 changes: 3 additions & 1 deletion source/cl/source/extension/include/extension/khr_command_buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,9 @@ struct _cl_command_buffer_khr final : public cl::base<_cl_command_buffer_khr> {
/// @param[in] mutable_config New configuration for one or more commands
///
/// @return CL_SUCCESS or appropriate OpenCL error code.
cl_int updateCommandBuffer(const cl_mutable_base_config_khr &mutable_config);
cl_int updateCommandBuffer(
cargo::array_view<const cl_mutable_dispatch_config_khr *>
&mutable_configs);

/// @brief Verifies whether a queue is compatible with the command-buffer.
///
Expand Down
30 changes: 13 additions & 17 deletions source/cl/source/extension/source/khr_command_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,19 +1072,15 @@ cargo::expected<mux_descriptor_info_s, cl_int> createArgumentDescriptor(
} // anonymous namespace

[[nodiscard]] cl_int _cl_command_buffer_khr::updateCommandBuffer(
const cl_mutable_base_config_khr &mutable_config) {
cargo::array_view<const cl_mutable_dispatch_config_khr *>
&mutable_dispatch_configs) {
const std::lock_guard<std::mutex> guard(mutex);
const cl_device_id device = command_queue->device;

const cargo::array_view<const cl_mutable_dispatch_config_khr>
mutable_dispatch_configs(mutable_config.mutable_dispatch_list,
mutable_config.num_mutable_dispatch);

// Verify struct configures kernel arguments and return error if malformed
for (const auto &config : mutable_dispatch_configs) {
OCL_CHECK(config.type != CL_STRUCTURE_TYPE_MUTABLE_DISPATCH_CONFIG_KHR,
return CL_INVALID_VALUE);

for (const auto config_ptr : mutable_dispatch_configs) {
OCL_CHECK(config_ptr == nullptr, return CL_INVALID_VALUE);
const cl_mutable_dispatch_config_khr config = *config_ptr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, config was a reference, it should probably remain a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this config variable a reference and the loop iteration variable config_ptr

OCL_CHECK(!config.command, return CL_INVALID_MUTABLE_COMMAND_KHR);
OCL_CHECK(config.command->command_buffer != this,
return CL_INVALID_MUTABLE_COMMAND_KHR);
Expand All @@ -1100,7 +1096,7 @@ cargo::expected<mux_descriptor_info_s, cl_int> createArgumentDescriptor(

for (auto config : mutable_dispatch_configs) {
unsigned update_index = 0;
const unsigned num_args = config.num_args + config.num_svm_args;
const unsigned num_args = config->num_args + config->num_svm_args;
UpdateInfo update_info;
if (update_info.descriptors.alloc(num_args)) {
return CL_OUT_OF_HOST_MEMORY;
Expand All @@ -1110,16 +1106,16 @@ cargo::expected<mux_descriptor_info_s, cl_int> createArgumentDescriptor(
return CL_OUT_OF_HOST_MEMORY;
}

if (update_info.pointers.alloc(config.num_svm_args)) {
if (update_info.pointers.alloc(config->num_svm_args)) {
return CL_OUT_OF_HOST_MEMORY;
}

const auto mutable_command = config.command;
const auto mutable_command = config->command;
update_info.id = mutable_command->id;
cargo::array_view<const cl_mutable_dispatch_arg_khr> args(config.arg_list,
config.num_args);
cargo::array_view<const cl_mutable_dispatch_arg_khr> args(config->arg_list,
config->num_args);

for (unsigned i = 0; i < config.num_args; ++i) {
for (unsigned i = 0; i < config->num_args; ++i) {
auto arg = args[i];
update_info.indices[update_index] = arg.arg_index;
auto descriptor =
Expand All @@ -1135,9 +1131,9 @@ cargo::expected<mux_descriptor_info_s, cl_int> createArgumentDescriptor(

#ifdef OCL_EXTENSION_cl_intel_unified_shared_memory
cargo::array_view<const cl_mutable_dispatch_arg_khr> svm_args(
config.arg_svm_list, config.num_svm_args);
config->arg_svm_list, config->num_svm_args);

for (unsigned i = 0; i < config.num_svm_args; ++i) {
for (unsigned i = 0; i < config->num_svm_args; ++i) {
// Unpack the argument.
const auto arg = svm_args[i];
const auto arg_index = arg.arg_index;
Expand Down
35 changes: 20 additions & 15 deletions source/cl/source/extension/source/khr_command_buffer_mutable_dispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ extension::khr_command_buffer_mutable_dispatch::
#else
usage_category::DISABLED
#endif
CA_CL_EXT_VERSION(0, 1, 0)) {
CA_CL_EXT_VERSION(0, 9, 2)) {
}

void *extension::khr_command_buffer_mutable_dispatch::
Expand Down Expand Up @@ -82,9 +82,10 @@ cl_int extension::khr_command_buffer_mutable_dispatch::GetDeviceInfo(
}

#ifdef OCL_EXTENSION_cl_khr_command_buffer_mutable_dispatch
CL_API_ENTRY cl_int CL_API_CALL
clUpdateMutableCommandsKHR(cl_command_buffer_khr command_buffer,
const cl_mutable_base_config_khr *mutable_config) {
CL_API_ENTRY cl_int CL_API_CALL clUpdateMutableCommandsKHR(
cl_command_buffer_khr command_buffer, cl_uint num_configs,
const cl_command_buffer_update_type_khr *config_types,
const void **configs) {
const tracer::TraceGuard<tracer::OpenCL> guard("clUpdateMutableCommandsKHR");
OCL_CHECK(!command_buffer, return CL_INVALID_COMMAND_BUFFER_KHR);
OCL_CHECK(!command_buffer->is_finalized, return CL_INVALID_OPERATION);
Expand All @@ -102,20 +103,24 @@ clUpdateMutableCommandsKHR(cl_command_buffer_khr command_buffer,
return CL_INVALID_OPERATION;
}

OCL_CHECK(!mutable_config, return CL_INVALID_VALUE);
OCL_CHECK(mutable_config->type != CL_STRUCTURE_TYPE_MUTABLE_BASE_CONFIG_KHR,
return CL_INVALID_VALUE);
OCL_CHECK(config_types && 0 == num_configs, return CL_INVALID_VALUE);
OCL_CHECK(!config_types && num_configs, return CL_INVALID_VALUE);

// Values for next would be defined by implementation of mutable mem commands
// layered extension. Later checks assume next is NULL and so the
// mutable_dispatch_list field must be set.
OCL_CHECK(mutable_config->next, return CL_INVALID_VALUE);
OCL_CHECK(configs && 0 == num_configs, return CL_INVALID_VALUE);
OCL_CHECK(!configs && num_configs, return CL_INVALID_VALUE);

OCL_CHECK(!mutable_config->mutable_dispatch_list ||
!mutable_config->num_mutable_dispatch,
return CL_INVALID_VALUE);
for (size_t i = 0; i < num_configs; i++) {
if (config_types[i] != CL_STRUCTURE_TYPE_MUTABLE_DISPATCH_CONFIG_KHR) {
return CL_INVALID_VALUE;
}
}

const cl_mutable_dispatch_config_khr **casted_configs =
reinterpret_cast<const cl_mutable_dispatch_config_khr **>(configs);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this (when dereferencing the result) is well-defined in standard C++, and I do not recall whether current compilers optimize on the assumption that we do not do this. Do you know?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're looking at the same thing, do you have a reference to where this is UB? I'm looking at https://en.cppreference.com/w/cpp/language/reinterpret_cast and guessing it's point 5) then "Type Accessibility" doesn't seem to cover this case as well defined.

I'm not sure how type based alias analysis works with void** that we're only reading from after the cast, could put this through compiler explorer to double check what some popular compilers are doing. An alternative could be to use a C-style case, but looking at https://en.cppreference.com/w/cpp/language/explicit_cast it seems like this will fallback to reinterpret_cast since static_cast isn't valid. The most conservative code is maybe defining a separate variable and memcpying to it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's the type accessibility thing. There is no need to memcpy, we can use cargo::array_view<const void *> and use a static_cast on its elements instead.

cargo::array_view<const cl_mutable_dispatch_config_khr *>
mutable_dispatch_configs(casted_configs, num_configs);

return command_buffer->updateCommandBuffer(*mutable_config);
return command_buffer->updateCommandBuffer(mutable_dispatch_configs);
}

CL_API_ENTRY cl_int CL_API_CALL clGetMutableCommandInfoKHR(
Expand Down
Loading
Loading