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] Implement Graph and node queries #348

Closed
wants to merge 18 commits into from
Closed

Conversation

Bensuo
Copy link
Collaborator

@Bensuo Bensuo commented Dec 20, 2023

Split off from #340

  • Add graph and node queries to the spec
  • Implement graph and node queries from spec
  • New node_type enum
  • Note, subgraph node type not yet implemented due to significant changes required
  • Explicit nodes now also have associated events (fixes mixed usage issue)
  • New tests for queries
  • Update ABI symbols

To better support mixed use cases, where for example the user records most of a graph through record and replay but only wants to update a few parameters on a small number of nodes, queries were added to get nodes from a graph to enable more mixed usage scenarios. Current options for mixed usage are limited to using handler::depends_on() with an event from a queue recorded node inside an explicit node to create a dependency. But giving the user access to all nodes in a graph enables use of make_edge() and the depends_on property as well. Included here is the bare mininum, but these queries could be expanded in future in response to user needs.

The queries to get nodes from a graph raises some questions about subgraphs and what we should be returning in that case. Currently the implementation actually copies these nodes directly into the parent graph and there is no single node that represents a subgraph, but this could cause confusion with users since the spec wording kind of implies that. Previously this wasn't an issue because that was just an implementation detail.

- Implement graph and node queries from spec
- New node_type enum
- Note, subgraph node type not yet implemented due to significant changes required
- Explicit nodes now also have associated events (fixes mixed usage issue)
- New tests for queries
- Update linux ABI symbols
@Bensuo Bensuo added Graph Specification Extension Specification related Graph Implementation Related to DPC++ implementation and testing labels Dec 20, 2023
Copy link
Collaborator

@mfrancepillois mfrancepillois left a comment

Choose a reason for hiding this comment

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

I think it might be good to test if we can depend on a node returned by get_node_from_event.
i.e. :

  • record a cmd
  • call get_node_from_event() with the returned event
  • add a new node that depend on the previous node using the explicit API

sycl/unittests/Extensions/CommandGraph.cpp Show resolved Hide resolved
sycl/source/handler.cpp Outdated Show resolved Hide resolved
sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@julianmi julianmi left a comment

Choose a reason for hiding this comment

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

LGTM. I agree that we might want to extend the testing.

sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved
Bensuo and others added 12 commits January 8, 2024 16:51
Commit b32db9a added some new suggested
text to the extension specification template, which was formatted as a
quote. This formatting is inconsistent with the other suggested text,
which is not formatted as a quote. Fix this.
…#12332)

This small addition signposts interested users to instructions on adding
DPC++ extensions. It was motivated by
intel#12251 (comment)
When looking into the logic of the docs, I found there was no
signposting from the initial repo readme to tell contributors how to
propose their own extensions.
There have been some important third-party contributions to dpc++ and in
the future it is possible such contributions could include proposing new
oneapi extensions.

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
…ntel#12351)

This change explicitly defines multi_ptr inside the kernel instead of
outside the kernel. Before, we rely on the runtime to correctly treat
the lambda capture clause and create correct private copies for each of
these variables. With this change, we are making sure that these
variables are correctly captured (private copies) inside the kernel.
…ies of the plugins (intel#12336)

Currently the default search order is used when loading dependencies of
the plugins (these dependencies include the Level Zero loader and the
ICD loader for opencl and level zero plugins respectively) and that list
includes current directory and some other directories which are not
considered safe. This patch limits the list of directories when loading
the dependencies of the plugins. See:
https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa
for reference.

---------

Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>
…11472)

- Adds support for fill and memset nodes in graphs.
- Supported on Level Zero only for now.
- Adds E2E and unit tests for these new node types.
- Minor modifications due to renaming of some UR functions.

---------

Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
Co-authored-by: Ewan Crawford <ewan@codeplay.com>
`fabs` was missing for Nvidia compilation
Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
Co-authored-by: ldrumm <ldrumm@rtps.co>
@Bensuo
Copy link
Collaborator Author

Bensuo commented Jan 11, 2024

Closing in favour of upstream PR: intel#12366

@Bensuo Bensuo closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graph Implementation Related to DPC++ implementation and testing Graph Specification Extension Specification related
Projects
None yet
Development

Successfully merging this pull request may close these issues.