Skip to content

[SYCL][DOC] Graph API extensions#5626

Merged
steffenlarsen merged 84 commits intointel:syclfrom
reble:sycl-graph-update
Jul 19, 2023
Merged

[SYCL][DOC] Graph API extensions#5626
steffenlarsen merged 84 commits intointel:syclfrom
reble:sycl-graph-update

Conversation

@reble
Copy link
Contributor

@reble reble commented Feb 22, 2022

This extension is adding an API that targets separating definition and execution of a SYCL Kernel Graph. This patch contains specification only.

@keryell
Copy link
Contributor

keryell commented Mar 5, 2022

Interesting concepts!
Perhaps the naming could be tweaked to be more SYCL-friendly?
Does the _node suffix really matter? It looks like this is what in SYCL we name a command group. But it could be seen as a task or similar.
add_ looks like submit so why not keeping the same wording? I guess you want to differentiate submit_device from submit_host, so adding _device and _host makes sense. But the host is also a device...
add_empty_node can be just an overload submit_device().

@jbrodman
Copy link
Contributor

But the host is also a device...

...not anymore. ;)

@keryell
Copy link
Contributor

keryell commented Mar 17, 2022

But the host is also a device...

...not anymore. ;)

You do not have an extension for this? :-)
It can be an optional feature in all the good implementations when a good CPU is available. ;-)

@reble reble force-pushed the sycl-graph-update branch from 5956e20 to d410644 Compare August 12, 2022 15:16
@reble reble changed the title [SYCL] Graph API extensions [SYCL][DOC] Graph API extensions Aug 12, 2022
@reble reble marked this pull request as ready for review August 12, 2022 17:09
@reble reble requested a review from a team as a code owner August 12, 2022 17:09
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

This sounds like an interesting feature also for SYCL SC.

Pablo Reble and others added 3 commits August 22, 2022 14:38
Co-authored-by: John Pennycook <john.pennycook@intel.com>
Co-authored-by: John Pennycook <john.pennycook@intel.com>
Co-authored-by: Ronan Keryell <ronan@keryell.fr>
- Make modifying kernel or host code in update undefined behaviour.
- Use host task consistently in spec
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple small items below.

EwanC pushed a commit to reble/llvm that referenced this pull request Jul 11, 2023
Simplify the definition of a node in the Record & Replay API.
Move the wording around sub-graphs to the sub-graph section,
and use "command" terminology rather than "kernel" to be
more generic.

Actions Gordon's feedback from:
* Lack of clarity over inclusion of queue shortcut functions
intel#5626 (comment)
* Say "command" rather than "kernel"
intel#5626 (comment)
Gordon and Greg left some spec feedback on our upstream PR, address the superficial comments.
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

Looks good!

EwanC pushed a commit to reble/llvm that referenced this pull request Jul 11, 2023
Address [Steffen's
feedback](intel#5626 (comment))
and move the `info::device::graph_support_level` to enum up a namespace
level to `info::graph_support_level`.

Using `info::device::graph_support` to return a `info::graph_support_level` is
analogous with the `info::device::device_type` query in the main
spec which returns a `info::device_type`. Or `info::device::local_mem_type`
which returns a `info::local_mem_type`.
to be inferred.

It is valid to combine these two mechanisms, however it is invalid to modify
a graph using the explicit API while that graph is currently being recorded to,
Copy link
Contributor

Choose a reason for hiding this comment

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

is it "... being recorded too". instead of ".. to" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that "to" rather than "too" is correct, but rephrased this wording to "it is invalid to modify a graph using the explicit API while that graph is currently recording commands" to avoid any confusion over grammar.

Ewan Crawford added 2 commits July 11, 2023 18:59
* [SYCL][Doc] Change `graph_support_level` namespace

Address [Steffen's
feedback](intel#5626 (comment))
and move the `info::device::graph_support_level` to enum up a namespace
level to `info::graph_support_level`.

Using `info::device::graph_support` to return a `info::graph_support_level` is
analogous with the `info::device::device_type` query in the main
spec which returns a `info::device_type`. Or `info::device::local_mem_type`
which returns a `info::local_mem_type`.

* Add Steffen as a contributor
Simplify the definition of a node in the Record & Replay API.
Move the wording around sub-graphs to the sub-graph section,
and use "command" terminology rather than "kernel" to be
more generic.

Actions Gordon's feedback from:
* Lack of clarity over inclusion of queue shortcut functions
intel#5626 (comment)
* Say "command" rather than "kernel"
intel#5626 (comment)
EwanC pushed a commit to reble/llvm that referenced this pull request Jul 12, 2023
[Review feedback](intel#5626 (comment))
questioned the phrase "graph is currently being recorded to" as to
whether "to" should be "too". I don't think it should be "too", but
rephrasing to "graph is currently recording any queues" avoids any
confusion.

Also updated the contributors to add Maxime and Jaime (who made the
comment).
[Review feedback](intel#5626 (comment))
questioned the phrase "graph is currently being recorded to" as to
whether "to" should be "too". I don't think it should be "too", but
rephrasing to "graph is currently recording any queues" avoids any
confusion.

Also updated the contributors to add Maxime and Jaime (who made the
comment).
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@steffenlarsen steffenlarsen requested a review from jandres742 July 12, 2023 13:59
- Change wording around undefined behaviour when creating a cycle with no checks
- UB is created at point of adding the cycle, not at finalize.
steffenlarsen pushed a commit that referenced this pull request Jul 17, 2023
…hs (3/4) (#10033)

# Backend integration and feature additions for SYCL Graphs
This is the third patch of a series that adds support for an
[experimental command graph
extension](#5626)

A snapshot of the complete work can be seen in draft PR #9375 which has
support all the specification defined ways of
adding nodes and edges to the graph, including both Explicit and Record
& Replay graph construction. The two types of nodes currently
implemented are kernel execution and memcpy commands.

See https://github.com/reble/llvm#implementation-status for the status
of our total work.

## Scope
This third patch focuses on integrating the graphs runtime with the
backend support added in #9992 as well as any remaining runtime features
and bug fixes, and should include no ABI-breaking changes:
* Graphs runtime changes to use PI/UR command-buffers.
* Various improvements to the Graphs runtime classes.
* New memory manager methods for appending copies to a command-buffer.
* Changes to the Scheduler and related CG classes to enable Graphs.
* Device info query for command-graph support.
* Minor changes to some runtime classes to enable Graphs.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* Add end-to-end tests for SYCL Graph extension. (4/4)
* NFC changes - Design doc and codeowner update.

## Authors
Co-authored-by: Pablo Reble <pablo.reble@intel.com>
Co-authored-by: Julian Miller <julian.miller@intel.com>
Co-authored-by: Ben Tracy <ben.tracy@codeplay.com>
Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Maxime France-Pillois
<maxime.francepillois@codeplay.com>
Copy link
Contributor

@jandres742 jandres742 left a comment

Choose a reason for hiding this comment

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

+1

@steffenlarsen
Copy link
Contributor

Precommit failure is a known issue (#10380)

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.