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

[SYCL][Graph] Adding graph support for enqueue function submit ... #15385

Closed

Conversation

reble
Copy link
Contributor

@reble reble commented Sep 13, 2024

... in explicit mode.

Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

Specification changes LGTM.

Copy link
Contributor

@jbrodman jbrodman left a comment

Choose a reason for hiding this comment

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

LGTM

@reble reble marked this pull request as ready for review September 13, 2024 15:18
@reble reble requested review from a team as code owners September 13, 2024 15:18
namespace sycl::ext::oneapi::experimental {

template <typename CommandGroupFunc>
void submit(command_graph<graph_state::modifiable> g, CommandGroupFunc&& cgf);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to correspond to command_graph::add? That's the API you use to build a graph in explicit mode. If that is the case, then the name of this API seems wrong. No commands are actually being submitted with this API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this maps to add. Naming is a compromise again between seamless integration into enqueue function extension and its current SYCL-like naming scheme: submit*.

Another option could be something like add_in_order or add_linear, indicating the implicit creation of edges.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this API implicitly create a linear graph? The corresponding function submit(sycl::queue q) doesn't imply a linear set of commands. Why should it be different when building a graph?

Regarding naming, how bout add_graph_node (assuming it does not have the linear semantic)?

Comment on lines +734 to +735
specification) to the `command_graph` for deferred execution. Subsequent calls
will create a linear chain of nodes with implicit edges.
Copy link
Contributor

@EwanC EwanC Sep 17, 2024

Choose a reason for hiding this comment

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

I'm not sure about these linear semantics, situations I can think of where this won't happen are

  1. if a buffer accessor is used in the command-group that could create edges to nodes to other predecessor.
  2. using handler::depends_on() in the command-group can create an edge.
  3. Using the explicit API that to create a node with more than one predecessor inbetween two calls to this API


}
----
!====
Copy link
Contributor

Choose a reason for hiding this comment

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

We've not defined any exceptions here, but I presume this would inherit the exceptions from the explicit graph.add() API. We should try link to that wording here.

Copy link
Contributor

@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.

Could you elaborate on what the benefits of this API are? I don't understand the motivation

It seems like the only difference to the current g.add() API is not returning a node object, which the user could ignore anyway. The benefits of sycl_ext_oneapi_enqueue_functions seem mostly related to avoiding the overhead of creating sycl events, which we don't do in the explicit API anyway.

@reble reble closed this Sep 18, 2024
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.

5 participants