[Command-Buffers] Implement new command-list enqueue path #1975

fabiomestre · 2024-08-14T17:54:48Z

Adds a new path that submits command-buffers using zeCommandListImmediateAppendCommandListsExp() instead of zeCommandQueueExecuteCommandLists(). This allows:

Waiting for command-buffer submission dependencies without having to create a separate command-list.
Since the WaitEvent is no longer needed in this path, counter-based events can be used for helper events which removes the need to create an event reset command-list.
When the command-buffer is not in-order, the new path also moves the reset event command-list to execute after the command-buffer execution.

Intel/llvm PR: intel/llvm#16096

source/adapters/level_zero/command_buffer.cpp

source/adapters/level_zero/command_buffer.hpp

source/adapters/level_zero/command_buffer.cpp

EwanC

Can you stick a link to the DPC++ PR in the description? (as part of that PR we might want to add an update to https://github.com/intel/llvm/blob/sycl/sycl/doc/design/CommandGraph.md#level-zero) to mention this new code path

source/adapters/level_zero/command_buffer.cpp

aarongreig

doc change looks fine to me

Adds a new path that uses the zeCommandListImmediateAppendCommandListsExp to submit command-buffers on PVC hardware.

pbalcer

L0 changes lgtm, just a few questions.

pbalcer · 2024-11-19T12:33:33Z

source/adapters/level_zero/command_buffer.cpp

@@ -22,6 +22,71 @@

 namespace {

+// Checks whether zeCommandListImmediateAppendCommandListsExp can be used for a
+// given Context and Device.
+bool checkImmediateAppendSupport(ur_context_handle_t Context,


longer-term, is there anything stopping us from deprecating the old non-immediate path? I don't like how the two modes are intermingling together. It makes the code more complex.

@EwanC , @reble ,

is @pbalcer right that old non-immediate path is not needed any more?

if so, can we remove non-immediate path right now?

One consideration is the supported driver versions, we need to make sure this works with the LTS driver. 30898 is relatively new.
So no, we can't just remove it, at least not yet.

We have an internal task to look into this in the future. At the moment it's not possible because it's still an experimental feature. It's also not clear if the implementation of zeCommandListImmediateAppendCommandListsExp has been properly tested on older GPU's and we still need to support those for Graphs.

Agree having 2 paths is definitely a maintenance burden, and I would very much like us to only have the new path in future. As has been said, in the short term we are blocked by:

LTS/public driver being recent enough to support this L0 feature

Verification on older GPUs for correctness/performance regressions.

pbalcer · 2024-11-19T12:39:09Z

source/adapters/level_zero/command_buffer.cpp

+    PrecondEvents.push_back(CommandBuffer->WaitEvent->ZeEvent);
+  }
+
+  ZE2UR_CALL(zeCommandListAppendBarrier,


Does this have to be a barrier? Can you use zeCommandListAppendWaitOnEvents instead?

The L0 documentation is not very clear. Could you clarify what are the advantages of zeCommandListAppendWaitOnEvents over a barrier with a nullptr signal event?

Does zeCommandListAppendWaitOnEvents provide the same guarantees as a barrier? This is, does it guarantee that any commands appended after the wait are not executed before the events are signalled?

zeCommandListAppendWaitOnEvents over a barrier with a nullptr signal event?

Barrier is coarse-grain and enforces global memory ordering.

Does zeCommandListAppendWaitOnEvents provide the same guarantees as a barrier? This is, does it guarantee that any commands appended after the wait are not executed before the events are signalled

Not for out-of-order cmdlists AFAIK. That is, commands can be reordered past AppendWaitOnEvents if there's no explicit event synchronization point between these two operations. But you are using events anyway, so that's why I've asked.

@nrspruit ping, in case I got anything wrong.

pbalcer · 2024-11-19T12:53:08Z

scripts/core/LEVEL_ZERO.rst

@@ -139,8 +139,15 @@ Environment Variables
 | UR_L0_DISABLE_USM_ALLOCATOR                 | Controls the use of the USM allocator.                       | "0": USM allocator is enabled.                               | "0"              |
 |                                             |                                                              | Any other value: USM allocator is disabled.                  |                  |
 +---------------------------------------------+--------------------------------------------------------------+--------------------------------------------------------------+------------------+
-
+| UR_L0_CMD_BUFFER_USE_IMMEDIATE_APPEND_PATH  | Controls which command-buffer implementation path is used.   | "1": the immediate append path will always be enabled as     | Unset            |


Which path is tested will depend on the CI runner. We should be more explicit about it by testing twice - with and without the "immediate append path" enabled.

At the moment, the CI drivers are all older than 30898 and I would assume that that will be the case for the foreseeable future. So it will always use the old path.

I think that, once this extension is not experimental anymore, if we are still using 2 paths in the code, we could add more testing to CI. But for now, I don't think there is much point in doing so.

AFAIK our PVC runners have a new enough driver (the newest available rolling is 24.39.31294.21), but fair, we can expand CI later. Please create an issue to track this.

Created ticket #2373

fabiomestre commented Aug 14, 2024

View reviewed changes

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

github-actions bot added level-zero L0 adapter specific issues command-buffer Command Buffer feature addition/changes/specification labels Aug 14, 2024

EwanC reviewed Aug 15, 2024

View reviewed changes

fabiomestre commented Aug 15, 2024

View reviewed changes

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

fabiomestre force-pushed the fabio/immediate_append_exp branch 3 times, most recently from 6c0b66c to a27ba98 Compare October 14, 2024 18:07

EwanC reviewed Oct 15, 2024

View reviewed changes

Bensuo reviewed Oct 15, 2024

View reviewed changes

fabiomestre force-pushed the fabio/immediate_append_exp branch from 24b6579 to 580bec9 Compare November 5, 2024 18:12

EwanC reviewed Nov 14, 2024

View reviewed changes

source/adapters/level_zero/command_buffer.cpp Show resolved Hide resolved

fabiomestre force-pushed the fabio/immediate_append_exp branch from 87ca82c to b67cd51 Compare November 14, 2024 18:59

github-actions bot added the specification Changes or additions to the specification label Nov 14, 2024

fabiomestre force-pushed the fabio/immediate_append_exp branch 2 times, most recently from 19c80b7 to 629dfb7 Compare November 14, 2024 19:15

fabiomestre marked this pull request as ready for review November 14, 2024 19:15

fabiomestre requested review from a team as code owners November 14, 2024 19:15

fabiomestre changed the title ~~Prototype new command-list submission entrypoint~~ Implement new command-list enqueue path Nov 14, 2024

fabiomestre changed the title ~~Implement new command-list enqueue path~~ [Command-Buffers] Implement new command-list enqueue path Nov 14, 2024

EwanC reviewed Nov 15, 2024

View reviewed changes

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

EwanC reviewed Nov 15, 2024

View reviewed changes

source/adapters/level_zero/command_buffer.cpp Show resolved Hide resolved

fabiomestre mentioned this pull request Nov 15, 2024

[SYCL][Graph] Implement new command-list enqueue path intel/llvm#16096

Open

aarongreig approved these changes Nov 15, 2024

View reviewed changes

Use zeCommandListImmediateAppendCommandListsExp

66c80c9

Adds a new path that uses the zeCommandListImmediateAppendCommandListsExp to submit command-buffers on PVC hardware.

fabiomestre force-pushed the fabio/immediate_append_exp branch from bb8bbe3 to 66c80c9 Compare November 15, 2024 17:33

EwanC approved these changes Nov 18, 2024

View reviewed changes

pbalcer self-requested a review November 18, 2024 15:31

pbalcer approved these changes Nov 19, 2024

View reviewed changes

Merge branch 'main' into fabio/immediate_append_exp

ca878c5

pbalcer added the ready to merge Added to PR's which are ready to merge label Nov 21, 2024

pbalcer merged commit 20e501a into oneapi-src:main Nov 21, 2024
74 of 75 checks passed

againull mentioned this pull request Nov 22, 2024

[UR] Update tag to get the fix for urProgramGetGlobalVariablePointer intel/llvm#16156

Merged

EwanC mentioned this pull request Nov 22, 2024

CI testing for UR_L0_CMD_BUFFER_USE_IMMEDIATE_APPEND_PATH #2373

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Command-Buffers] Implement new command-list enqueue path #1975

[Command-Buffers] Implement new command-list enqueue path #1975

fabiomestre commented Aug 14, 2024 •

edited

Loading

EwanC left a comment

aarongreig left a comment

pbalcer left a comment

pbalcer Nov 19, 2024

lslusarczyk Nov 20, 2024

pbalcer Nov 20, 2024

fabiomestre Nov 20, 2024

EwanC Nov 21, 2024

pbalcer Nov 19, 2024

fabiomestre Nov 20, 2024

pbalcer Nov 21, 2024

pbalcer Nov 19, 2024 •

edited

Loading

fabiomestre Nov 20, 2024

pbalcer Nov 21, 2024 •

edited

Loading

EwanC Nov 22, 2024

[Command-Buffers] Implement new command-list enqueue path #1975

[Command-Buffers] Implement new command-list enqueue path #1975

Conversation

fabiomestre commented Aug 14, 2024 • edited Loading

EwanC left a comment

Choose a reason for hiding this comment

aarongreig left a comment

Choose a reason for hiding this comment

pbalcer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbalcer Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbalcer Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiomestre commented Aug 14, 2024 •

edited

Loading

pbalcer Nov 19, 2024 •

edited

Loading

pbalcer Nov 21, 2024 •

edited

Loading