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] Add node and graph queries for mixed usage #12366

Merged
merged 18 commits into from
Feb 15, 2024

Conversation

Bensuo
Copy link
Contributor

@Bensuo Bensuo commented Jan 11, 2024

This PR adds queries to both nodes and modifiable graphs which enable better mixed usage of both the explicit and record & replay APIs in a single program.

It also reworks how subgraphs are handled: previously nodes were merged into the modifiable graph, but this would pose a problem for users querying the graph since they would not see a single subgraph node, and this merging behaviour was an implementation detail. This has been changed so that now subgraph nodes are only merged in the executable graph, and are stored as a single node of type subgraph in the modifiable graph.

As a consequence of this change all nodes are now also copied when making the executable graph, where previously they were not.

  • Reworked how subgraphs are handled
  • Add graph and node queries to the SYCL-Graph spec
  • Implement graph and node queries
  • New node_type enum
  • Explicit nodes now also have associated events (fixes mixed usage issue)
  • New tests for queries
  • Update ABI symbols

- Subgraphs are now stored in modifiable graphs as a single node, instead of being merged
- Executable graphs copy all nodes internally (pre-partitioning) and merge subgraph nodes into the main node list
- Subgraph merging no longer uses empty nodes
- Subgraph nodes now use CGExecCommandBuffer instead of being empty
- CGExecCommandBuffer now stores exec graph (to pass to node)
- Modifications to unit tests due to internals changing
- Added dot printing for exec_graph_impl as an internal debugging feature
@Bensuo Bensuo marked this pull request as ready for review January 18, 2024 10:39
@Bensuo Bensuo requested review from a team as code owners January 18, 2024 10:39
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.

Initial comments, will do another review pass for exec_graph_impl::duplicateNodes() since I need to come at that with fresh eyes.

sycl/source/detail/graph_impl.hpp Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
sycl/source/handler.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
@EwanC
Copy link
Contributor

EwanC commented Jan 24, 2024

ping @intel/llvm-reviewers-runtime for review

@EwanC
Copy link
Contributor

EwanC commented Jan 31, 2024

ping @intel/llvm-reviewers-runtime

sycl/include/sycl/detail/cg.hpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Show resolved Hide resolved
sycl/source/detail/graph_impl.cpp Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
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.

One small nit, but otherwise I think it looks good.

sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
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!

@Bensuo
Copy link
Contributor Author

Bensuo commented Feb 14, 2024

@intel/llvm-gatekeepers Can this be merged?

@sommerlukas sommerlukas merged commit 5337a8a into intel:sycl Feb 15, 2024
12 checks passed
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.

6 participants