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.

Sign up for GitHub

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

Jump to bottom

[EXP][Command-Buffer] Support for using copy queue #10

Closed
wants to merge 3 commits into from
Closed

[EXP][Command-Buffer] Support for using copy queue #10

wants to merge 3 commits into from

Conversation

mfrancepillois
Copy link

@mfrancepillois mfrancepillois commented Mar 15, 2024 •
edited by Bensuo
Loading

  • Add another command-lists for copy operations.
  • If a main copy engine is available on the device, memory operations are enqueued in the copy command-list while
    the other commands are enqueued in the general command-list.
  • On submission, if not empty, the copy command-list is sent to the main copy command-queue while the general command-list is sent to the compute command-queue.
  • Both are executed concurrently. Synchronization between the command-lists is handled by Level-Zero events.
  • Because of this the optimization is not currently available with in-order command-lists.
  • This functionality respects the following existing L0 environment variables:
    • UR_L0_USE_COPY_ENGINE_FOR_FILL
    • UR_L0_USE_COPY_ENGINE_FOR_D2D_COPY

@mfrancepillois
Copy link
Author

DPC++ linked PR: reble/llvm#362

@omarahmed1111
[CTS] Add CTS tests for urMemImageCreate entry-point
92fce2e 
* Adds CTS tests for different image types.
* Adds CTS tests for different image formats
* Defines the primary and optional supported image formats.
* Deletes redundant ERROR_IMAGE_FORMAT_NOT_SUPPORTED error code.
* Deletes UR_MEM_TYPE_BUFFER as it should be redundant to urMemBufferCreate entry-point.
* Deletes URMEM_TYPE_IMAGE1D_BUFFER which is only used in opencl and opencl will need cl_image_desc.buffer/mem_object value when CL_MEM_OBJECT_IMAGE1D_BUFFER is used, and we don't have a way in urMemImageCreate to pass it a buffer object that it will use to initialize an image with it and use this type. And if we initialized a new buffer inside the entry-point, we don't have a way of clearing the buffer memory at the end.
* Make sure the adapters return the right error-codes for urMemImageCreate entry-point.
EwanC
EwanC reviewed May 31, 2024
View reviewed changes
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Rebasing this on tip UR would help review

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.hpp Outdated Show resolved Hide resolved
source/adapters/level_zero/command_buffer.hpp Outdated Show resolved Hide resolved
source/adapters/level_zero/command_buffer.cpp Show resolved Hide resolved
source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved
@EwanC
Copy link
Collaborator

EwanC commented May 31, 2024

@fabiomestre Pointed out that there's environment variables in oneapi-src#1528 that mention copy engines, and whether we should guard when we do this optimization based on those.

@Bensuo
Copy link
Owner

Bensuo commented Jun 3, 2024

@fabiomestre Pointed out that there's environment variables in oneapi-src#1528 that mention copy engines, and whether we should guard when we do this optimization based on those.

I don't think currently we respect any of these. From a quick look:

  • UR_L0_USE_COPY_ENGINE - Needs some more investigation to understand what this does and how we would best support it, curently I think we'll only use the copy engine if there is a single main engine available.
  • UR_L0_USE_COPY_ENGINE_FOR_FILL & UR_L0_USE_COPY_ENGINE_FOR_D2D_COPY - These we could respect pretty easily.
  • UR_L0_USE_COPY_ENGINE_FOR_IN_ORDER_QUEUE - This one could be a bit confusing to respect, since the in-order-ness of the UR queue doesn't necessarily affect the command-buffer being in-order. It is enabled by default, so perhaps we should make it the default also, but if we wanted to have this option I'd probably argue for a command buffer specific option. This is in contrast to the current behaviour in this PR which only uses copy engines for out-of-order command lists in command-buffers.

These do seem like mostly performance/debugging tweaks so I don't know that respecting them is super high priority, The Fill/D2D copy ones should be pretty trivial to implement though.

@Bensuo Bensuo force-pushed the cmd-buf-copy-queue branch 2 times, most recently from 97cb4a1 to 46fa4ee Compare June 4, 2024 14:20
EwanC
EwanC approved these changes Jun 4, 2024
View reviewed changes
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM

@kbenzie
Merge pull request oneapi-src#1409 from omarahmed1111/Add-CTS-tests-f…
4f10526 
…or-image-format

[CTS] Add CTS tests for urMemImageCreate entry-point
@Bensuo
[EXP][CMD-BUF][L0] Use copy engine to optimize cmd-buffer usage
76f84a1 
- Out-of-order command-buffers now use copy-engine in some cases where available
- Also respect existing L0 env vars for using copy-engine with fill and d2d copies
@Bensuo
Copy link
Owner

Bensuo commented Jun 11, 2024

Closed in favor of upstream PR: oneapi-src#1738

@Bensuo Bensuo closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EwanC EwanC EwanC approved these changes

@fabiomestre fabiomestre fabiomestre approved these changes

@ori-sky ori-sky Awaiting requested review from ori-sky

@reble reble Awaiting requested review from reble

@Bensuo Bensuo Awaiting requested review from Bensuo

@julianmi julianmi Awaiting requested review from julianmi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mfrancepillois @EwanC @Bensuo @fabiomestre @omarahmed1111 @kbenzie