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][DOC] Graph API extensions #5626

Merged
merged 84 commits into from
Jul 19, 2023
Merged

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

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

* [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 added 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!

- 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)

@steffenlarsen steffenlarsen merged commit f9d4830 into intel:sycl Jul 19, 2023
1 check passed
EwanC added a commit to EwanC/llvm that referenced this pull request Jul 19, 2023
After the merge of the [SYCL-Graph extension spec PR](intel#5626)
add @intel/sycl-graphs-reviewers as reviewers of the specification
[document](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_graph.asciidoc).

Used `**` for the extension status directory as the extension
is expected to shortly move from proposed to experimental.
steffenlarsen pushed a commit that referenced this pull request Jul 28, 2023
# E2E Tests for SYCL Graphs
This is the fourth 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 fourth patch focuses on adding E2E tests for SYCL Graphs, covering
the following:

* Record and Replay API based tests.
* Explicit API based tests.
* Thread safety tests.
* A small amount of miscellaneous tests.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* NFC changes - Design doc.

## 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>
veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 21, 2023
# Level Zero Backend Support for SYCL Graphs
This is the second patch of a series that adds support for an
[experimental command graph
extension](intel#5626)

A snapshot of the complete work can be seen in draft PR intel#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 second patch focuses on the required PI/UR support for the
experimental command-buffer feature in the Level Zero adapter:
* PI stubs for all adapters to enable compilation, no functionality.
* Command-buffer implementation for the Level Zero UR adapter.
* Stubs for the CUDA UR adapter to enable compilation, no functionality.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* Hooking up backend to graphs runtime, bugfixes and other feature
additions, will add symbols but not break the ABI. (3/4)
* 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>

---------

Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 21, 2023
…hs (3/4) (intel#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](intel#5626)

A snapshot of the complete work can be seen in draft PR intel#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 intel#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>
fabiomestre pushed a commit to fabiomestre/llvm that referenced this pull request Sep 26, 2023
# Level Zero Backend Support for SYCL Graphs
This is the second patch of a series that adds support for an
[experimental command graph
extension](intel#5626)

A snapshot of the complete work can be seen in draft PR intel#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 second patch focuses on the required PI/UR support for the
experimental command-buffer feature in the Level Zero adapter:
* PI stubs for all adapters to enable compilation, no functionality.
* Command-buffer implementation for the Level Zero UR adapter.
* Stubs for the CUDA UR adapter to enable compilation, no functionality.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* Hooking up backend to graphs runtime, bugfixes and other feature
additions, will add symbols but not break the ABI. (3/4)
* 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>

---------

Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
fabiomestre pushed a commit to fabiomestre/unified-runtime that referenced this pull request Sep 26, 2023
# Level Zero Backend Support for SYCL Graphs
This is the second patch of a series that adds support for an
[experimental command graph
extension](intel/llvm#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 second patch focuses on the required PI/UR support for the
experimental command-buffer feature in the Level Zero adapter:
* PI stubs for all adapters to enable compilation, no functionality.
* Command-buffer implementation for the Level Zero UR adapter.
* Stubs for the CUDA UR adapter to enable compilation, no functionality.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* Hooking up backend to graphs runtime, bugfixes and other feature
additions, will add symbols but not break the ABI. (3/4)
* 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>

---------

Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
fabiomestre pushed a commit to oneapi-src/unified-runtime that referenced this pull request Sep 27, 2023
# Level Zero Backend Support for SYCL Graphs
This is the second patch of a series that adds support for an
[experimental command graph
extension](intel/llvm#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 second patch focuses on the required PI/UR support for the
experimental command-buffer feature in the Level Zero adapter:
* PI stubs for all adapters to enable compilation, no functionality.
* Command-buffer implementation for the Level Zero UR adapter.
* Stubs for the CUDA UR adapter to enable compilation, no functionality.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* Hooking up backend to graphs runtime, bugfixes and other feature
additions, will add symbols but not break the ABI. (3/4)
* 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>

---------

Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
# E2E Tests for SYCL Graphs
This is the fourth patch of a series that adds support for an
[experimental command graph
extension](intel#5626)

A snapshot of the complete work can be seen in draft PR intel#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 fourth patch focuses on adding E2E tests for SYCL Graphs, covering
the following:

* Record and Replay API based tests.
* Explicit API based tests.
* Thread safety tests.
* A small amount of miscellaneous tests.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* NFC changes - Design doc.

## 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>
omarahmed1111 pushed a commit to omarahmed1111/unified-runtime that referenced this pull request Oct 23, 2023
# Level Zero Backend Support for SYCL Graphs
This is the second patch of a series that adds support for an
[experimental command graph
extension](intel/llvm#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 second patch focuses on the required PI/UR support for the
experimental command-buffer feature in the Level Zero adapter:
* PI stubs for all adapters to enable compilation, no functionality.
* Command-buffer implementation for the Level Zero UR adapter.
* Stubs for the CUDA UR adapter to enable compilation, no functionality.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* Hooking up backend to graphs runtime, bugfixes and other feature
additions, will add symbols but not break the ABI. (3/4)
* 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>

---------

Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
omarahmed1111 pushed a commit to omarahmed1111/unified-runtime that referenced this pull request Oct 23, 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](intel/llvm#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>
omarahmed1111 pushed a commit to omarahmed1111/unified-runtime that referenced this pull request Oct 23, 2023
# Level Zero Backend Support for SYCL Graphs
This is the second patch of a series that adds support for an
[experimental command graph
extension](intel/llvm#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 second patch focuses on the required PI/UR support for the
experimental command-buffer feature in the Level Zero adapter:
* PI stubs for all adapters to enable compilation, no functionality.
* Command-buffer implementation for the Level Zero UR adapter.
* Stubs for the CUDA UR adapter to enable compilation, no functionality.

## Following Split PRs
Future follow-up PRs with the remainder of our work on the extension
will include:
* Hooking up backend to graphs runtime, bugfixes and other feature
additions, will add symbols but not break the ABI. (3/4)
* 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>

---------

Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
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.