Skip to content

[SYCL][L0] Use immediate commandlists. #5833

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

Merged
merged 33 commits into from
Apr 15, 2022
Merged

[SYCL][L0] Use immediate commandlists. #5833

merged 33 commits into from
Apr 15, 2022

Conversation

rdeodhar
Copy link
Contributor

@rdeodhar rdeodhar commented Mar 17, 2022

This change adds a mode where the plugin uses immediate commandlists instead of standard commandlists. The default remains standard commandlists. The new mode is selected by setting an environment variable.

Signed-off-by: Rajiv Deodhar [email protected]

@rdeodhar rdeodhar marked this pull request as ready for review March 22, 2022 21:12
@rdeodhar rdeodhar requested a review from a team as a code owner March 22, 2022 21:12
@smaslov-intel
Copy link
Contributor

I took a brief look and looks great overall!

The batching tests are, of course, expected to fail, and we need to:

  1. Run them with batching forced
  2. Add new test for immediate submission

Additionally, we need to

  1. Make use of device-scope events (I suggest doing so in separate PR, and force all host-visible in this PR)
  2. Measure perf impact to different HW (and chose presumably different defaults)
  3. Document new option (in debug section)

@rdeodhar rdeodhar requested a review from a team as a code owner March 23, 2022 06:27
@rdeodhar rdeodhar requested a review from asudarsa March 24, 2022 17:21
@bader bader changed the title [SYCL] [L0] Use immediate commandlists. [SYCL][L0] Use immediate commandlists. Mar 24, 2022
@bader bader requested a review from smaslov-intel March 25, 2022 15:25
@smaslov-intel
Copy link
Contributor

There is existing _pi_context::ZeCommandListInit immediate command-list that we now use for buffer initialization. I think that can be changed to use the new infra instead.

@rdeodhar
Copy link
Contributor Author

rdeodhar commented Apr 5, 2022

"There is existing _pi_context::ZeCommandListInit immediate command-list that we now use for buffer initialization. I think that can be changed to use the new infra instead."

That commandlist uses ZE_COMMAND_QUEUE_MODE_SYNCHRONOUS for immediate initialization. Other commandlists do not use this mode so are not directly usable.

@smaslov-intel
Copy link
Contributor

That commandlist uses ZE_COMMAND_QUEUE_MODE_SYNCHRONOUS for immediate initialization. Other commandlists do not use this mode so are not directly usable.

True. Let's not touch it then.

@rdeodhar rdeodhar requested a review from smaslov-intel April 6, 2022 05:03
@smaslov-intel
Copy link
Contributor

This change switches from using ordinary commandlists to immediate commandlists.

It just add this mode, but doesn't switch to it by default, right? Please update the description.

smaslov-intel
smaslov-intel previously approved these changes Apr 8, 2022
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@againull againull merged commit b9cb1d1 into intel:sycl Apr 15, 2022
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.

5 participants