[SYCL][Graph] Add local memory parameter update functionality#16712
[SYCL][Graph] Add local memory parameter update functionality#16712fabiomestre wants to merge 6 commits intointel:syclfrom
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 |
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
EwanC
changed the title
- Specify new usage for dynamic parameters after compiler support is added. - Update get() function to be used only in device code. - Remove set_arg() overloads - Clarify template parameters limitations and add static_asserts to class definition. - Fix wording of work_group_memory parameters.
|
Let me know when you want me to review this again. I saw you pushed another commit, but I wasn't sure if you have more to come. |
I pushed another commit after that but it's only updating the usage guide with a few more examples. Feel free to review the PR again. I think I addressed all the unresolved comments. |
gmlueck
left a comment
gmlueck
left a comment
There was a problem hiding this comment.
I think this looks really good. I have one minor comment below, but I'm approving anyway. If you agree with my comment, it would be nice to address it before merging,
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
Updates the dynamic parameter class to require underlying type to be device copyable.
- Implements the dynamic_local_accessor class with compiler support. - Refactor the recently added dynamic_work_group_memory class to only use one `impl` member variable. This brings it closer to the design of other sycl classes and avoids future ABI break issues. - There are 2 ABI breaking changes. However, they are both related to the `dynamic_work_group_memory` class whose [specification](#16712) has not been merged yet and is not yet officially supported.
|
@reble I will be closing this PR as I am no longer part of the organization. Feel free to use the contents as basis for future work. |
2 participants
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.