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.

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

Add local memory parameter update functionality to sycl graphs. #382

Draft
wants to merge 4 commits into
base: sycl
Choose a base branch
from

Conversation

fabiomestre
Copy link
Collaborator

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.

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.
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Comments are all trivial editorial details from a pass reading this over

|===

===== The dynamic_work_group_memory class [[dynamic-work-group-memory-class]]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor but a quick sentence linking to the sycl_ext_oneapi_work_group_memory extension doc again here would be handy.

Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

This looks good, we could maybe also have an examples in sycl/doc/syclgraph/SYCLGraphUsageGuide.md to help show the new dynamic classes and aid stakeholder review.

buffer<DataT, 1, AllocatorT> &bufferRef,
const property_list &propList = {})
----
|Available only when `(Dimensions == 0)``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

double backtick at end (and in other places)


Parameters:

* `handler` - The kernel handler that represents the current submission.
Copy link
Collaborator

Choose a reason for hiding this comment

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

param is called cgh rather than handler (same comment for other dynamic classes)

|
[source,c++]
----
local_accessor<DataT, Dimensions> get(handler &cgh);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if you call get() on a handler without doing set_arg()? Do we need an exception for that (which could be relaxed later once we have compiler support)

Copy link
Collaborator Author

@fabiomestre fabiomestre Jan 17, 2025

Choose a reason for hiding this comment

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

I think calling get() without set_arg() should just give you a regular local_accessor that doesn't get updated on a call to update()

I think this would be more consistent with the current pattern we have with dynamic_parameters where set_arg() cannot be enforced.

Adding an exception is not hard though if we think that is better. A consequence of throwing an exception is that get() should always be called afer set_arg() but that's probably not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants