-
Notifications
You must be signed in to change notification settings - Fork 733
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specification changes LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
namespace sycl::ext::oneapi::experimental { | ||
|
||
template <typename CommandGroupFunc> | ||
void submit(command_graph<graph_state::modifiable> g, CommandGroupFunc&& cgf); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
specification) to the `command_graph` for deferred execution. Subsequent calls | ||
will create a linear chain of nodes with implicit edges. |
There was a problem hiding this comment.
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
- if a buffer accessor is used in the command-group that could create edges to nodes to other predecessor.
- using
handler::depends_on()
in the command-group can create an edge. - Using the explicit API that to create a node with more than one predecessor inbetween two calls to this API
|
||
} | ||
---- | ||
!==== |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
... in explicit mode.