Support for cl_mem_device_address_EXT #742
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kpet
reviewed
Dec 1, 2024
|
|
||
|
|
||
| #endif // cl_ext_buffer_device_address | ||
|
|
There was a problem hiding this comment.
I have discussed releasing this extension with the Khronos OpenCL working group. The authors seem motivated to make it happen. In the meantime, I suggest you move all definitions that will be provided by the headers to src/cl_headers.hpp.
| @@ -303,6 +328,7 @@ static const std::unordered_map<std::string, void*> gExtensionEntrypoints = { | |||
| EXTENSION_ENTRYPOINT(clCreateCommandQueueWithPropertiesKHR), | |||
| EXTENSION_ENTRYPOINT(clGetKernelSuggestedLocalWorkSizeKHR), | |||
| {"clGetKernelSubGroupInfoKHR", FUNC_PTR(clGetKernelSubGroupInfo)}, | |||
| {"clSetKernelArgDevicePointerEXT", FUNC_PTR(clSetKernelArgDevicePointerEXT_fn)}, | |||
There was a problem hiding this comment.
Suggested change
| {"clSetKernelArgDevicePointerEXT", FUNC_PTR(clSetKernelArgDevicePointerEXT_fn)}, | |
| {"clSetKernelArgDevicePointerEXT", FUNC_PTR(clSetKernelArgDevicePointerEXT)}, |
For consistency with other extensions. You could then just use EXTENSION_ENTRYPOINT I think.
| ret = CL_INVALID_OPERATION; | ||
| break; | ||
| } | ||
| val_sizet = buffer->device_address(); |
There was a problem hiding this comment.
The spec defines a new type that aliases cl_ulong which, contrary to size_t is guaranteed to have a size of 64 bits.
| #endif | ||
|
|
||
| typedef cl_ulong cl_mem_device_address_EXT; | ||
|
|
There was a problem hiding this comment.
Please move these definitions to cl_headers.hpp for now.
| @@ -47,6 +47,9 @@ struct cvk_platform; | |||
|
|
|||
| struct cvk_device : public _cl_device_id, | |||
| object_magic_header<object_magic::device> { | |||
| /// Map for storing device pointers to buffer pointers | |||
| /// Support cl_ext_buffer_device_address | |||
| std::unordered_map<void*, void*> device_to_buffer_map; | |||
There was a problem hiding this comment.
Those pointers would only be valid for a given context. I think we probably want to keep this state in cvk_context. Please use the specific types instead of void*.
| @@ -626,6 +626,7 @@ void cvk_device::build_extension_ils_list() { | |||
| // Add always supported extensions | |||
| MAKE_NAME_VERSION(1, 0, 0, "cl_khr_extended_versioning"), | |||
| MAKE_NAME_VERSION(1, 0, 0, "cl_khr_create_command_queue"), | |||
| MAKE_NAME_VERSION(1, 0, 0, "cl_ext_buffer_device_address"), | |||
There was a problem hiding this comment.
The current spec drafts does not define 1.0.0, only 0.1.0, 0.2.0, and 0.3.0.
|
|
||
| std::cout << "All tests completed successfully\n"; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
I'm guessing the intention is to move all the test code to tests/api/buffer_device_address.cpp, isn't it?
| // device pointer found in map, swapping the buffer pointer | ||
| auto buffer_ptr_raw = it->second; | ||
| auto buffer_ptr = reinterpret_cast<cvk_buffer*>(buffer_ptr_raw); | ||
| m_kernel_resources[arg.binding] = buffer_ptr; |
There was a problem hiding this comment.
This will break the existing argument setting for buffers. It might be easier/cleaner to introduce a new cvk_kernel::set_arg_device_address function (that would have to also mark the argument as set as done by line 361).
kpet
reviewed
Dec 1, 2024
Comment on lines
+29
to
+41
| // First check if device supports the extension | ||
| size_t ext_size; | ||
| GetDeviceInfo(CL_DEVICE_EXTENSIONS, 0, nullptr, &ext_size); | ||
|
|
||
| std::vector<char> extensions(ext_size); | ||
| GetDeviceInfo(CL_DEVICE_EXTENSIONS, ext_size, extensions.data(), nullptr); | ||
|
|
||
| bool hasBufferDeviceAddress = | ||
| std::string(extensions.data()).find("cl_ext_buffer_device_address") != std::string::npos; | ||
|
|
||
| if (!hasBufferDeviceAddress) { | ||
| GTEST_SKIP() << "Device does not support cl_ext_buffer_device_address extension"; | ||
| } |
There was a problem hiding this comment.
Suggested change
| // First check if device supports the extension | |
| size_t ext_size; | |
| GetDeviceInfo(CL_DEVICE_EXTENSIONS, 0, nullptr, &ext_size); | |
| std::vector<char> extensions(ext_size); | |
| GetDeviceInfo(CL_DEVICE_EXTENSIONS, ext_size, extensions.data(), nullptr); | |
| bool hasBufferDeviceAddress = | |
| std::string(extensions.data()).find("cl_ext_buffer_device_address") != std::string::npos; | |
| if (!hasBufferDeviceAddress) { | |
| GTEST_SKIP() << "Device does not support cl_ext_buffer_device_address extension"; | |
| } | |
| // First check if device supports the extension | |
| REQUIRE_EXTENSION("cl_ext_buffer_device_address"); |
This is being introduced by #748 for another test.
|
@kpet thank you for the review, I'll work on addressing your comments. Meanwhile, this is what I get when I try to execute an example: I'm not an LLVM expert but just wanted to check with you first before I investigate further. |
|
Can you share the kernel source and the command line? Everything will be in clvk's log, that you would get running with: then upload |
|
The kernel source is a HIP matrix multiplication @rjodinchr |
|
The error message you have posted is about |
|
Oh, I understand what you meant by:
This application is using OpenCL SPIR-V source. This was not clear to me as SPIR-V has 2 variant. The OpenCL one and the Vulkan one. While the goal of clvk is to compile whatever CL source it gets to Vulkan SPIR-V, it can take OpenCL SPIR-V as the input. But the log does not contain the OpenCL SPIR-V inputs. Could you share the sources then (I see two calls to |
|
Hmm only one got dumped for me: |
|
I'll try to reproduce with that one then. Thank you |
|
I'm not able to reproduce. That should produce a |
|
|
So the issue is that We can force This is because we have the following line in the OpenCL SPIR-V kernel: Which gets translated into the following LLVM IR (by Note that it is on Removing every trace of |
|
|
|
Disabled it, and I was able to run a HIP example on Vulkan! |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
still WIP, sample works but test fails though more tests fail even on main