[SYCL][Graph] Add local memory parameter update functionality #16712
Conversation
Updates the sycl graph specification to add the dynamic_accessor, dynamic_local_accessor and dynamic_work_group_memory classes. This adds the required functionality to support updating local memory parameters to sycl graph kernel nodes. Additionally, it also moves the accessor update functionality from the dynamic_parameter class to the new dynamic_accessor class. This improves the cohesion of the API and removes the need to use placeholder accessors when updating buffer arguments in sycl graphs.
|
@gmlueck These are the proposed spec changes for the new dynamic classes. It would be great if you could review them. There is also a PR that implements |
| access_mode AccessMode = | ||
| (std::is_const_v<DataT> ? access_mode::read | ||
| : access_mode::read_write), | ||
| target AccessTarget = target::device> |
There was a problem hiding this comment.
The only other non-deprecated access target is target::host_task. Can you use dynamic_accessor with a host task? If not, maybe we should remove this template parameter.
There was a problem hiding this comment.
The initial implementation of dynamic_accessor will probably not support accessors with target::host_task. However, we think that adding support for updating accessors in host tasks is something that we will eventually do. Given how hard it is to change the ABI after the initial implementation, we would prefer to future proof the dynamic_accessor class by keeping the target::host_task template parameter. I could update the specification to mention that this is not implemented yet and throw an exception on the implementation. Would this be an acceptable solution?
There was a problem hiding this comment.
OK, that makes sense.
I could update the specification to mention that this is not implemented yet and throw an exception on the implementation.
Let's word the spec to say that AccessTarget must be target::device. I think we can implement this as a static_assert, rather than a runtime check.
| graph which contains the registered nodes. The new value will not be reflected in any | ||
| executable graphs created from that modifiable graph until `command_graph::update()` | ||
| is called passing the modified nodes, or a new executable graph is finalized from | ||
| the modifiable graph. |
There was a problem hiding this comment.
What happens if the dynamic_accessor was originally constructed with a range and/or offset? I presume this call to update will eliminate them, so the accessor points to the beginning of the new buffer? It would be good to clarify this behavior in the description.
There was a problem hiding this comment.
Yes, this call to update() would override the range and/or offset that were passed during construction and point to the beginning of the buffer. I will update the spec to make this more clear.
| buffer<DataT, 1, AllocatorT> &bufferRef, | ||
| const property_list &propList = {}) | ||
| ---- | ||
| |Available only when `(Dimensions == 0)`. |
There was a problem hiding this comment.
I'm going to make an argument that you don't need these constraints on Dimensions.
This overload is exactly the same as the one below that has constraint Dimension > 0. Therefore, I think you can just remove the constraint and combine the two overloads.
The next overload with TagT (and no range or offset) makes perfect sense also in the case when Dimension == 0, so why prevent it?
The remaining overloads all take either range<Dimensions> or id<Dimensions>. It is impossible to create a range<0> or an id<0>, so the constraint doesn't really mean anything for these overloads.
There was a problem hiding this comment.
The dynamic_accessor class is essentially a wrapper around the accessor class. So, when writing this spec, I tried to stay consistent with the existing constructors and descriptions in the sycl spec: https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#accessor
While I do agree that there might be better ways to design these constructors, and that some of them could be combined, I think it's more important to be consistent
with the existing spec design and wording to avoid creating unnecessary confusion. Similarly, if at some point we update the wording in the sycl spec for the original accessor class, those changes should be reflected in the dynamic_accessor class as well. Does that seem reasonable to you?
| command_graph<graph_state::modifiable> graph, | ||
| range<Dimensions> allocationSize); | ||
| ---- | ||
| |Available only when `(Dimensions > 0)`. |
There was a problem hiding this comment.
Rather than making this a constraint on the constructor, should there just be a requirement for the class that Dimensions is 1, 2, or 3? How do we implement the the restriction that Dimensions is not greater than 3? Is this a static_assert in the class definition?
There was a problem hiding this comment.
What you suggest makes sense but I'm still a bit reluctant to deviate too much from the original spec for the local_accessor class. I think it might be a bit confusing.
The implementation there seems to rely on static_assert so we could add something similar. Adding it to the class deifnition makes sense since it makes the error more clear.
There was a problem hiding this comment.
Either way is OK with me. I'll point out, though, that you have already deviated from the local_accessor spec by omitting the constructor that works for Dimension == 0. It makes sense to omit that -- I'm not suggesting that you add it back. Since you omitted that constructor, the constraint here that Dimension > 0 is a little weird because there is no alternate constructor that takes Dimension == 0.
| }; | ||
| } | ||
| ---- | ||
|
|
There was a problem hiding this comment.
I think there should be a requirement that DataT is an unbounded array type, right? It seems like the only thing you can update is the element count, and that's only a runtime value in the case when the type is an unbounded array.
If you agree, this might be a static_assert in the class definition. The issue is similar to the Dimensions requirement I mention above for dynamic_local_accessor.
There was a problem hiding this comment.
Yes, I agree, the other data types don't make sense. I will add the unbounded array restriction and static_assert to the class definition.
| Parameters: | ||
|
|
||
| * `graph` - Graph which will contain the nodes that use the dynamic work group memory. | ||
| * `num` - The number of `DataT` elements in the dynamic work group memory. |
There was a problem hiding this comment.
This wording is not quite right. I think DataT is an unbounded array type in this case, so num specifies the number of elements in that array, not the number of DataT elements in the dynamic_work_group_memory object.
There was a problem hiding this comment.
I will update the wording to try to make this more clear 👍
| Registration happens inside the command-group that the node | ||
| represents, and is done when the dynamic parameter is set as a parameter to the | ||
| kernel using `handler::set_arg()`/`handler::set_args()`. It is valid for a node | ||
| argument to be registered with more than one dynamic parameter instance. |
There was a problem hiding this comment.
Just making sure we are on the same page ...
You can only use set_arg / set_args to set kernel arguments for a free function kernel. Therefore, I think the current specification only supports dynamic parameters for free function kernels. If you agree, I think we should somehow make this more clear in the spec.
In the future, I think we could support dynamic parameters in normal kernels by simply capturing the dynamic_parameter (etc.) object in the lambda.
We should also be a bit more formal about the declaration of the parameter in the kernel function. With dynamic_parameter, I think people were constructing the dynamic_parameter object on the host and passing it to handler::set_arg. However, I think people, declared the type of the argument in the free function kernel as the DataT type, not as dynamic_parameter<DataT>.
For these new types like dynamic_work_group_memory, it seems like the parameter type in the free function kernel should be dynamic_work_group_memory<DataT>, not just DataT. Is that also your expectation?
This difference is not documented in the specification, and the inconsistency seems confusing. Should we change dynamic_parameter to be consistent with the others, requiring the application to define the kernel parameter type as dynamic_parameter<DataT>?
If we do this, I think we need to add a get member function to dynamic_parameter, which returns DataT& (or maybe DataT). That would also make dynamic_parameter more consistent with the other types.
There was a problem hiding this comment.
-
Yes, the only officially supported way of using
set_argsis with free function kernels. I will update the wording to make that more clear. However, at the moment, free function kernels don't seem to supportlocal_accessororaccessorclasses. So we are using the "unofficial" way for internal testing. Otherwise, we have no way to test this new functionality. -
I was assuming that, with free function kernels, the type would be that of the underlying object (e.g.
work_group_memory). Theget()member function requires ahandlerso I think that it cannot be used inside free function kernels. Without compiler support for the dynamic classes, I think that using the underlying type as a parameter is the only way for it to work. The specification for free function kernels is a bit vague though, so maybe I could be missing something? -
Once we have compiler support, and
get()functions are no longer required, maybe capturing the dynamic objects in lambdas and using the dynamic classes in free function kernels would be the most consistent way to do things. -
Adding a
get()member function to thedynamic_parameterclass makes sense to me. It makes all the dynamic classes more consistent with each other.
There was a problem hiding this comment.
However, at the moment, free function kernels don't seem to support
local_accessororaccessorclasses.
This might be a reason to not implement dynamic_local_accessor or dynamic_accessor right now. I don't think anyone is asking for them currently.
Without compiler support for the dynamic classes, I think that using the underlying type as a parameter is the only way for it to work. The specification for free function kernels is a bit vague though, so maybe I could be missing something?
Oh, I missed that get takes a handler parameter. I thought the main reason for adding get was to call it from inside the kernel. In fact, it seems dangerous to me to allow get to be called from host code. What is the use case for get as it is defined now?
My thinking was that the user would pass dynamic_accessor (etc.) to set_arg, and also define the kernel to take a parameter of type dynamic_accessor. The kernel would then call get inside the kernel to get an accessor, and the kernel would use the accessor as normal.
The nice thing about this code pattern is that it will also work when the kernel is not a free function.
| dynamicParamAcc) | ||
| template <typename T> | ||
| void handler::set_arg(int argIndex, | ||
| ext::oneapi::experimental::dynamic_parameter<T> &dynamicParam); |
There was a problem hiding this comment.
Why do we need these special overloads of set_arg? The core SYCL specification defines these with an unconstrained template parameter, so you can call them with any kernel parameter type, including dynamic_parameter.
There was a problem hiding this comment.
The wording and behaviour of set_arg() in the core SYCL specification is different from the one in the dynamic classes. We could specialize the existing template for the dynamic types but, from an implementation point of view, I'm not sure if that approach would have any advantage compared to what we are doing at the moment. And from a specification point of view, it makes it harder to tell the reader about the special behaviour and exceptions that exist when using set_args() with dynamic classes.
There is also some precedent for using overloads since the raw_kernel_arg extension is doing the same.
EwanC
changed the title
Updates the sycl graph specification to add the dynamic_accessor, dynamic_local_accessor and dynamic_work_group_memory classes.
This adds the required functionality to support updating local memory parameters to sycl graph kernel nodes.
Additionally, it also moves the accessor update functionality from the dynamic_parameter class to the new dynamic_accessor class. This improves the cohesion of the API and removes the need to use placeholder accessors when updating buffer arguments in sycl graphs.