-
Notifications
You must be signed in to change notification settings - Fork 736
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 new graph enqueue function to spec #15677
base: sycl
Are you sure you want to change the base?
Conversation
sycl/doc/extensions/experimental/sycl_ext_oneapi_enqueue_functions.asciidoc
Show resolved
Hide resolved
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.
Do we anticipate the need to pass any compile-time properties to these methods in future?
This PR modifies the |
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 should probably update the wording on the extension interaction in the SYCL-Graph spec to mention this new API https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc#71213-sycl_ext_oneapi_enqueue_functions
---- | ||
namespace sycl::ext::oneapi::experimental { | ||
|
||
void submit(sycl::queue q, command_graph<graph_state::executable> g); |
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.
This is not consistent with the way we structured the other APIs in this extension that submit work. I think it would be more consistent to add:
namespace sycl::ext::oneapi::experimental {
void graph_launch(sycl::queue q, command_graph<graph_state::executable> g);
void graph_launch(sycl::handler h, command_graph<graph_state::executable> g);
}
I'm not sure about the name "graph_launch", but we should choose some name that makes it clear you are enqueuing a graph. Maybe @Pennycook has some name suggestions.
There is no need to add something like graph_launch_with_event
. Our thinking is that applications can use the handler
version if they want an event like this:
sycl::event e = syclex::submit_with_event(q, [&](sycl::handler& h) {
syclex::graph_launch(h, g);
});
Also, should the command_graph
be passed by reference? Maybe this is better:
void graph_launch(sycl::queue q, command_graph<graph_state::executable>& g);
void graph_launch(sycl::handler h, command_graph<graph_state::executable>& g);
I see that you use pass by reference in "sycl_ext_oneapi_graph" such as handler::ext_oneapi_graph(command_graph<graph_state::executable>& graph)
.
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 agree that we should try to avoid "launch". sycl_khr_free_function_commands is trending towards using launch
as a replacement for parallel_for
, and launch_graph
would not be consistent with that usage.
I'm not sure whether we want to use a noun or a verb here, as we have a mix (e.g., compare command_barrier
and copy
). Some possible names off the top of my head, all based on the idea that what we're submitting is a request to execute a specific graph:
graph_execution
orexecute_graph
execution
orexecute
(idea here would be to allow overloads for other things we deem "executable" in the future)
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.
Looking at sycl_khr_free_function_commands, it seems like "launch_graph" would be a good name. That would give us:
launch
-- Launch a simple "range" kernellaunch_grouped
-- Launch an "nd-range" kernellaunch_task
-- Launch a "single-task" kernellaunch_graph
-- Launch a graph
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 disagree. I don't think launch
in the first three cases means "enqueue some work"; we're semantically tying "launch" to launching work-items and the execution of a kernel:
launch
- Launch a specified number of work-items.launch_grouped
- Launch a specified number of work-items, but the work-items are grouped together.launch_task
- Launch a single work-item (perhapslaunch_single
/launch_one
would be a better name).
Given the above, I think somebody would expect launch_graph
to launch a single kernel, where a graph somehow describes the operations performed within a single kernel. (I'm not sure what that would be, exactly, but I don't think my argument depends on that.) The function we're trying to name doesn't launch a single kernel -- it submits a graph comprising multiple commands (some of which may be launch
commands) to the device.
Incidentally, this is why I proposed aligning with bulk
in the original draft. launch
has the potential to confuse in a way that bulk
doesn't; if the most basic operation was called bulk
, nobody would be suggesting that we call this bulk_graph
. Whatever word(s) we pick to replace the basic parallel_for
and the command "run this embarrassingly parallel operation on a bunch of work-items", I feel strongly that we shouldn't re-use that word to describe other things.
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.
OK.
It seems like we use verbs more often than nouns in that KHR. I'm not sure I like the generic execute
function name, which depends on overloads to determine what is being launched. We didn't seem to follow that pattern with launch
, launch_gruped
, etc., where we have separate names for each type of launch.
How about execute_graph
?
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.
execute_graph
works for me.
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.
Thanks for your feedback.
I have a preference for the overload option, just execute
, because a command_graph object is already strongly typed. It would be easier to use and is less verbose.
Since consistency is more important, I'm fine with the execute_graph
naming option.
void execute_graph(sycl::queue q, command_graph<graph_state::executable>& g);
void execute_graph(sycl::handler h, command_graph<graph_state::executable>& g);
replacing #15385 after offline discussion.