cl_ext_buffer_device_address #1159
Conversation
|
any motivation to get this merged? Or anything else needed to discuss before merging this? Could also try to bring it up at the CL WG if needed. |
Yep, this is still being discussed in the WG. I personally think it's useful as is and shouldn't harm anything if merged as it even has 2 implementations now. |
|
Thanks @SunSerega |
|
Alright, and now the problem I found in #1171 is visible here because the |
This comment was marked as resolved.
This comment was marked as resolved.
Yes, this was the idea. I'll add a mention in the next update. |
|
Updated according to @karolherbst comments. |
There was a problem hiding this comment.
I've already implemented the extension fully in rusticl/mesa (including sharing the same address across devices) and I think it's fine, however I'd still urge to address the concerns I have for layered implementations implementing it on top of Vulkan. I've already considered the constraints when implementing it, however I think it's better to provide clients to query if address sharing across multiple devices is supported or not.
pjaaskel
force-pushed
the
cl_ext_buffer_device_address
branch
from
b8df46b to
1931416
Compare
|
@SunSerega thanks! |
|
@karolherbst I asked about this in the CL/memory WG. We need to submit CTS tests and this might be good to go then with this one. Do you have good tests in Rusticl side we could use? The test in PoCL is quite basic (and needs to be updated), but can be used as a starting point also. |
I haven't written any more tests and only used the one in pocl. But I can probably help out with writing tests here. |
pjaaskel
force-pushed
the
cl_ext_buffer_device_address
branch
from
9ad04fb to
edcff73
Compare
|
Thanks @kpet for the feedback. Implemented most of it. |
pjaaskel
force-pushed
the
cl_ext_buffer_device_address
branch
from
edcff73 to
c07ff8f
Compare
|
After @karolherbst and @kpet are happy with this, we'll implement in PoCL and @franz will add a CTS pull request. Then we can mark this 1.0.0 and merge, I think. |
|
Right. I'll add the other option, returning the SVM pointer in this case, in the next specification revision. |
|
When adding the sentence about SVM, is this also ready to be marked 1.0.0 and merged in (wondering should I do it with the same commit)? |
|
No concerns in regards to that from my side. I think from a technical perspective it's in a good shape to land, though I don't want to rule out that more clarifications might be needed once others implement it as well. |
pjaaskel
force-pushed
the
cl_ext_buffer_device_address
branch
from
f9d2828 to
6e8cbe4
Compare
|
I cleaned up the commit history and the history description in the docs and upped it to 1.0.0. The headers generated OK. IMHO we could merge this in and update the CTS next. |
pjaaskel
force-pushed
the
cl_ext_buffer_device_address
branch
from
6e8cbe4 to
676312c
Compare
pjaaskel
force-pushed
the
cl_ext_buffer_device_address
branch
from
2219171 to
e5fa28a
Compare
|
Good points @bashbaug, I changed these and made it rev 1.0.1. Pinging @karolherbst @franz |
The basic cl_mem buffer API doesn't enable access to the underlying raw pointers in the device memory, preventing its use in host side data structures that need pointer references to objects. This API adds a minimal increment on top of cl_mem that provides such capabilities.
Also made the enums globally unique.
The only content addition since the previous version is
"If the device supports SVM and {clCreateBufferWithProperties} is
called with a pointer returned by {clSVMAlloc} as its _host_ptr_
argument, and {CL_MEM_USE_HOST_PTR} is set in its _flags_ argument,
the device-side address is guaranteed to match the _host_ptr."
* Made it explicit that passing illegal pointers is legal as long as they are not referenced. * Removed CL_INVALID_ARG_VALUE as a possible error in clSetKernelArgDevicePointerEXT() as there are no illegal pointer cases when calling this function. Return CL_INVALID_OPERATION for clGetMemObjectInfo() if the pointer is not a buffer device pointer. * clSetKernelExecInfo() and clSetKernelArgDevicePointerEXT() now only error out if no devices in the context associated with kernel support device pointers.
Converted the clSetKernelArgDevicePointerEXT() address parameter to a value instead of a pointer to the value.
pjaaskel
force-pushed
the
cl_ext_buffer_device_address
branch
from
2ef0476 to
8d13bfa
Compare
|
I believe I've now addressed the feedback. @franz pls check that this 1.0.2 matches the PoCL implementation and the CTS. |
|
Would it be possible to create a draft PR with headers for this extension, generated from the XML file in this PR? Sometimes I've found that that can be a good way to identify XML issues, and we'll need to do this anyhow before merging the CTS changes. |
|
Ah, I was planning to simply attach the generated cl_ext.h but forgot. Would this work: |
I can attach a diff, because I'll need one for mesa anyway. |
|
Here's a draft PR with header changes generated from the XML file in this PR: KhronosGroup/OpenCL-Headers#273 |
|
Discussed in the February 11th teleconference. Are these spec changes ready to go? In order to merge the CTS changes we need to merge the header changes first, and in order to merge the header changes we really ought to merge the XML changes first, which is included in this PR. There are a few unresolved conversations in the PR, but I think many of them are close to a resolution. |
|
This is ready to go as far as I'm concerned. I forgot to resolve the conversations, but I addressed those in the latest commits. |
|
Great. @kpet you still have some requested changes, but I think they've been addressed? Let's see if we can get this merged in next week's call, if not before. If we can get a review approval on the headers then we can merge the headers, also. KhronosGroup/OpenCL-Headers#273 |
The basic cl_mem buffer API doesn't enable access to the underlying raw pointers in the device memory, preventing its use in host side data structures that need pointer references to objects. This API adds a minimal increment on top of cl_mem that provides such capabilities.
The version 0.1.0 is implemented in PoCL and rusticl for prototyping, but everything's still up for discussion. chipStar is the first client that uses the API.