[SYCL][Graph] Add initial support for SYCL Graph (1/4) #9728

reble · 2023-06-02T21:47:17Z

Initial SYCL-Graph Patch

This is the first patch of a series that adds support for an experimental command graph extension

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 first patch focuses on ABI breaking changes and includes:

Experimental implementation of extension API (hpp and cpp).
New sycl::handler constructor and graph related member variables.
New scheduler command type CG::CGTYPE::ExecCommandBuffer.
Change CGExecKernel::MHostKernel member variable from a unique ptr to a shared ptr so that the command-group can be copied.
Extensions unittests tests.

Following Split PRs

Future follow-up PRs with the remainder of our work on the extension will include:

Add UR support with Level Zero implementation. (2/4)
Changes to the runtime not covered here, such as bugfixes and feature additions, will add symbols but not break the ABI. (3/4)
Add test-e2e 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

sycl/source/detail/graph_impl.cpp

EwanC · 2023-06-06T08:31:53Z

Codeplays contributions to this work aren't attributed in the initial split commit of this PR, @EwanC (ewan@codeplay.com) & @Bensuo (ben.tracy@codeplay.com) should be named as co-authors in the merge commit when this goes in.

The current approach uses the same LO PI event for all submissions of the same graph, and also doesn't use the wait events to enforce dependencies on the command-list submission. By doing these in the L0 adapter, we can remove the blocking queue wait from our graphs submission code in the runtime. Closes issue #139

bader · 2023-06-07T03:35:38Z

Codeplays contributions to this work aren't attributed in the initial split commit of this PR, @EwanC (ewan@codeplay.com) & @Bensuo (ben.tracy@codeplay.com) should be named as co-authors in the merge commit when this goes in.

FYI - https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#merge
@reble, @EwanC, all the commits in this PR will be squashed to a single commit before the merge, and commit message will be comprised from the PR title and description. Please, make sure that attributions are added to the PR description instead of the initial commit.

bader

Part one of the code review.
Thanks a lot for the comments in the code. They are very helpful.

sycl/include/sycl/queue.hpp

sycl/include/sycl/detail/cg.hpp

bader · 2023-06-07T04:16:46Z

sycl/source/detail/queue_impl.hpp

@@ -32,6 +32,8 @@
 #include <sycl/queue.hpp>
 #include <sycl/stl.hpp>

+#include "detail/graph_impl.hpp"


NIT: technically, we can use forward declaration of graph_impl instead of including the header with the full declaration. In this header we only use it as shared_ptr type.
Using forward declaration will improve compile time. As this header is not included to the user's code, this change will help to build the DPC++ runtime faster, so it impacts only DPC++ developers, but not DPC++ users.
Similar to the approach applied to sycl/include/sycl/handler.hpp.

Addressed in reble#212

bader · 2023-06-07T04:20:40Z

sycl/source/detail/scheduler/commands.cpp

-    // MHostKernel will be used elsewhere.
-    std::unique_ptr<HostKernelBase> *CopyHostKernel =
-        new std::unique_ptr<HostKernelBase>(std::move(HostTask->MHostKernel));
+    std::shared_ptr<HostKernelBase> CopyHostKernel = HostTask->MHostKernel;



I'd like someone from the runtime team to confirm that this change is okay.

Is there somebody in particular, or a group, we can tag for this?

@intel/llvm-reviewers-runtime is the team, but I would especially like @sergey-semenov's thoughts on this and the deep-copies and copy-ctor on CG.

Linking to Ben's comment on this in another resolved thread for reference https://github.com/intel/llvm/pull/9728/files#r1222976913

This change seems fine to me. As for CG copies, we might be able to share some parts like MArgsStorage between them (assuming my understanding that arguments are guaranteed to stay the same is correct).

sycl/include/sycl/ext/oneapi/experimental/graph.hpp

bader · 2023-06-07T05:03:53Z

sycl/include/sycl/ext/oneapi/experimental/graph.hpp

+  /// An executable command-graph is not user constructable.
+  command_graph() = delete;
+
+  /// Constructor used by internal runtime.


Why the modifiable command_graph constructor is private, whereas the executable command_graph constructor is public?

Addressed in reble#219

sycl/source/detail/graph_impl.cpp

sycl/source/handler.cpp

EwanC · 2023-06-07T08:31:25Z

If we pull in the code from these two commits it should fix two of the Windows CI fails:

It might be that the alignment part of 194 is stale though, and needs redone to reflect new upstream changes.

Addresses https://github.com/intel/llvm/pull/9728/files#r1222089289

With our change from storing a host-task in a command-group to a shared-pointer from a unique shared-pointer we didn't cover all the usages. When the native kernel is called in `DispatchNativeKernel`, the `void*` argument is currently casted to a unique_ptr and then freed at the end of the function. However, we set this `void*` to a raw pointer from the shared-pointer `get()`. This results in freeing the underlying memory held by the shared-pointer in `DispatchNativeKernel` before the shared-pointer's reference count is decremented to zero. I initially tried removing the `delete` in `DispatchNativeKernel`, which fixes `Basic/handler/run_on_host_intel.cpp` but doesn't fix `Basic/handler/run_on_host_intel2.cpp`, because user code can result in the command-group being deleted before the native kernel is run, causing the shared-pointer to decrement to a zero reference count and free the memory. The proposed fix in this patch creates a shared-pointer on the heap every time the host-task is enqueued, this increments the reference count of the shared-pointer for the lifetime it takes for the `DispatchNativeKernel` callback to run, and the heap shared-pointer is then freed at the end of `DispatchNativeKernel`. Fixes the following test-e2e tests for OpenCL CPU: ``` Basic/handler/run_on_host_intel.cpp Basic/handler/run_on_host_intel2.cpp DiscardEvents/discard_events_host_task.cpp ```

* [SYCL][Graph] Update scheduler regresion test to use shared_ptr An identical fix to #214 that was missed as part of #226 * Use shared_pointer in `CGExecKernel` constructor

`test/abi/sycl_symbols_windows.dump` was invalidated by the last commit, regenerate from script

dm-vodopyanov · 2023-06-16T18:20:00Z

Failure in post-commit:
https://github.com/intel/llvm/actions/runs/5292836543/jobs/9580214816
https://github.com/intel/llvm/actions/runs/5292836543/jobs/9580212367
@reble please, fix as soon as possible.

Together with [PR#9949](#9949): Fix post-commit failures after [PR#9728](#9728)

This commit attempts to fix the exported symbols after intel#9728 by creating templateless base classes for the different graph class specializations. This is in line with how other classes export their symbols. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

This commit attempts to fix the exported symbols after #9728 by creating templateless base classes for the different graph class specializations. This is in line with how other classes export their symbols. --------- Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

The SYCL-Graphs extension code which landed in intel#9728 is a collaboration between Codeplay and Intel. When graphs specific code is touched, the teams across both Intel and Codeplay should be notified. For example, in intel#9977 no Codeplay developers were tagged (Not that we have any issues with that PR).

reble added 3 commits June 2, 2023 13:28

initial split

16fc23a

remove remaining command buffer traces

51252d1

Merge branch 'sycl' into sycl-graph-release-patch1

44c438b

reble mentioned this pull request Jun 2, 2023

[SYCL][Graph] Adding experimental implementation of SYCL Command Graph extension #9375

Closed

fix clang format messed by merge

77b5787

reble temporarily deployed to aws June 2, 2023 23:11 — with GitHub Actions Inactive

fix issues intr. by merge

f99add6

reble temporarily deployed to aws June 3, 2023 04:44 — with GitHub Actions Inactive

reble added the abi-break change that's breaking abi and waiting for the next window to be able to merge label Jun 5, 2023

reble marked this pull request as ready for review June 6, 2023 04:11

reble requested a review from a team as a code owner June 6, 2023 04:11

reble requested a review from againull June 6, 2023 04:11

EwanC reviewed Jun 6, 2023

View reviewed changes

sycl/source/detail/graph_impl.cpp Outdated Show resolved Hide resolved

reble and others added 5 commits June 6, 2023 07:23

update linux symbol reference

c930c7a

[SYCL][Graph] Bugfix empty cgf explicit mode (#207)

a6f2485

update symbol refs and layout for abi tests

ef71ba3

clang-format

28fa544

againull requested review from steffenlarsen, aelovikov-intel, sergey-semenov and bader June 6, 2023 16:51

reble temporarily deployed to aws June 6, 2023 17:44 — with GitHub Actions Inactive

Merge branch 'sycl' into pablo/sycl-graph-release-patch1-rebase

5ae93e3

bader changed the title ~~[SYCL][Graph] Initial support for SYCL Graph (1/3)~~ [SYCL][Graph] Add initial support for SYCL Graph (1/3) Jun 7, 2023

bader reviewed Jun 7, 2023

View reviewed changes

reble temporarily deployed to aws June 7, 2023 06:59 — with GitHub Actions Inactive

EwanC temporarily deployed to aws June 13, 2023 08:05 — with GitHub Actions Inactive

Merge branch 'sycl' into sycl-graph-release-patch1

7ab1d0d

EwanC temporarily deployed to aws June 13, 2023 08:48 — with GitHub Actions Inactive

EwanC temporarily deployed to aws June 13, 2023 10:20 — with GitHub Actions Inactive

EwanC temporarily deployed to aws June 13, 2023 10:59 — with GitHub Actions Inactive

[SYCL][Graph] Including more feedback from PR (#222)

85a649b

Addresses https://github.com/intel/llvm/pull/9728/files#r1222089289

reble changed the title ~~[SYCL][Graph] Add initial support for SYCL Graph (1/3)~~ [SYCL][Graph] Add initial support for SYCL Graph (1/4) Jun 14, 2023

[SYCL][Graph] Make internal constructor private (#219)

16ca50e

reble temporarily deployed to aws June 14, 2023 21:31 — with GitHub Actions Inactive

reble and others added 5 commits June 14, 2023 19:38

Merge branch 'sycl' into sycl-graph-release-patch1

6953c1f

Update queue.hpp

ae54b1a

clang format queue.hpp

164560f

adapt changes from intel#9837

4f21586

reble temporarily deployed to aws June 15, 2023 03:31 — with GitHub Actions Inactive

[SYCL][Graph] Update scheduler regresion test to use shared_ptr (#228)

af8e79d

* [SYCL][Graph] Update scheduler regresion test to use shared_ptr An identical fix to #214 that was missed as part of #226 * Use shared_pointer in `CGExecKernel` constructor

EwanC temporarily deployed to aws June 15, 2023 12:49 — with GitHub Actions Inactive

[SYCL][Graph] Update windows symbol tests

2b30beb

`test/abi/sycl_symbols_windows.dump` was invalidated by the last commit, regenerate from script

EwanC temporarily deployed to aws June 15, 2023 13:26 — with GitHub Actions Inactive

EwanC temporarily deployed to aws June 15, 2023 14:17 — with GitHub Actions Inactive

reble requested a review from steffenlarsen June 15, 2023 15:35

againull approved these changes Jun 16, 2023

View reviewed changes

againull merged commit 35465da into intel:sycl Jun 16, 2023

reble mentioned this pull request Jun 18, 2023

[SYCL][Graph] Fix unannotated fall-through between switch labels #9966

Merged

steffenlarsen pushed a commit that referenced this pull request Jun 19, 2023

[SYCL][Graph] Fix unannotated fall-through between switch labels (#9966)

1a6f7a5

Together with [PR#9949](#9949): Fix post-commit failures after [PR#9728](#9728)

steffenlarsen mentioned this pull request Jun 19, 2023

[SYCL] Make base classes for graph states #9977

Merged

[SYCL][Graph] Add initial support for SYCL Graph (1/4) #9728

[SYCL][Graph] Add initial support for SYCL Graph (1/4) #9728

Uh oh!

Conversation

reble commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Initial SYCL-Graph Patch

Scope

Following Split PRs

Authors

Uh oh!

Uh oh!

EwanC commented Jun 6, 2023

Uh oh!

bader commented Jun 7, 2023

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EwanC commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dm-vodopyanov commented Jun 16, 2023

Uh oh!

Uh oh!

reble commented Jun 2, 2023 •

edited

Loading

EwanC commented Jun 7, 2023 •

edited

Loading