[SYCL][Graph] Blocking wait in finalize on scheduler node dependencies #335
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Bensuo Could you check if this patch works on CUDA, which we added the original asynchronous wait for |
Ben pulled this change into our open upstream CUDA PR intel#11133 and the CI passed |
EwanC
commented
Oct 24, 2023
EwanC
force-pushed
the
ewan/graph_scheduling_blocking_wait
branch
from
17e89c6
to
35456ac
Compare
EwanC
changed the title
EwanC
force-pushed
the
ewan/graph_scheduling_blocking_wait
branch
from
35456ac
to
b0333a1
Compare
reble
reviewed
Oct 25, 2023
EwanC
force-pushed
the
ewan/graph_scheduling_blocking_wait
branch
from
b0333a1
to
0f92801
Compare
Bensuo
approved these changes
Oct 26, 2023
julianmi
approved these changes
Oct 26, 2023
When command-groups uses accessors in SYCL-Graph nodes, our implementation uses the scheduler to wrangle dependencies and then add the node to the graph. This happens at the application level during graph finalization. We currently have a call to `waitForEvents()` when adding nodes, that waits on the device event dependencies of that node command-group. For example, a memory copy command. However, this corresponds to a call to `piEnqueueWaitForEvents`(), which is an asynchronous call that itself returns an event that must be waited on later. The blocking wait on this returned event is invoked by the scheduler on enqueue of the executable graph. This patch changes this behaviour to perform blocking waits earlier, on node addition to the PI/UR command-buffer during graph finalization. We have designed `finalize()` as the computationally expensive entry-point in the extension spec, so blocking behaviour makes sense here rather than on executable graph submission. There are two categories of blocking wait done when the scheduler adds a node to the graph: 1. Blocking wait on host event dependencies, these correspond to scheduler commands that don't have an associated PiEnqueue call. For example, memory allocation commands in the scheduler. Adding this wait fixes the E2E failures on DG2 devices, so the tests are re-enabled. 2. Blocking wait on device event dependencies, these correspond to scheduler commands that do have an associated PiEnqueue call that returns an event. These can occur when regular queue submissions are interleaved with adding graph nodes. Introducing this wait fixes fails in the `buffer_ordering.cpp` E2E test on some Level Zero devices.
EwanC
force-pushed
the
ewan/graph_scheduling_blocking_wait
branch
from
0f92801
to
6f5064d
Compare
|
Closing this, and moving it to an upstream PR intel#11706 I also checked if these changes are still required after intel#11691, and looks like they are. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When command-groups uses accessors our SYCL-Graph implementation uses the scheduler to wrangle dependencies and add nodes to the graph. This happens during graph finalization. We currently have a call to
waitForEventswhen adding nodes, to wait on the device events dependencies of that node command-group. For example, a memory copy command.However, this corresponds to
piEnqueueWaitForEvents()which is an asynchronous call, and itself returns an event that must be waited on later. The blocking wait on this returned event is invoked by the scheduler on enqueue of the executable graph.This patch changes this behaviour to perform blocking waits earlier, on node enqueue during graph finalization. We have specified
finalize()as the computationally expensive entry-point, so blocking behaviour makes sense here rather than on graph submission.There are two categories of blocking wait done here:
Blocking wait on host event dependencies, these correspond to scheduler commands that don't have an associated PiEnqueue call. For example, memory allocation commands in the scheduler. Adding this wait fixes the E2E failures on DG2 devices, so the tests are re-enabled.
Blocking wait on device event dependencies, these correspond to scheduler commands that do have an associated PiEnqueue call that returns an event. These can occur when regular queue submissions are interleaved with adding graph nodes. Introducing this wait fixes fails in the
buffer_ordering.cppE2E test on some Level Zero devices.Note I'd open this PR upstream once the standalone UR bump PR is merged