Skip to content

[SYCL] Use scheduler in queue shortcuts to avoid waiting for deps #11758

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 52 commits into from
Jan 29, 2024

Conversation

KseniyaTikhomirova
Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova commented Nov 2, 2023

Some queue shortcuts attempt to bypass the scheduler to avoid the
overhead of creating a command node. In cases where dependencies
couldn't be passed to the backend directly, there was a blocking wait
during submission.

This patch changes that behavior to bypass the scheduler only if all
dependency events can be used directly: host task dependencies have been
completed, others have been enqueued and there are no cross-context
dependencies.

Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Copy link
Contributor

@mfrancepillois mfrancepillois left a comment

Choose a reason for hiding this comment

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

Sycl-Graph LGTM

@sergey-semenov
Copy link
Contributor

@aelovikov-intel Could you please have another look?

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

Stylistically it's good, the comments are very helpful as well.

I've reviewed the logic to the best of my abilities/knowledge in the area but the latter is very limited and I might have missed something. That said, you're the expert in the area and I don't know if anybody available could help with the review, so let's merge it.

@sergey-semenov
Copy link
Contributor

Last commit was a comment typo fix, merging based on the previous test run.

@martygrant
Copy link
Contributor

@KseniyaTikhomirova @sergey-semenov there is a post-commit failure for this PR which should be addressed, as the error has occurred in a more recent PR. The errors are:

/Users/runner/work/llvm/llvm/src/sycl/source/detail/queue_impl.hpp:747:64: error: unused parameter 'Type' [-Werror,-Wunused-parameter] void finalizeHandler(HandlerType &Handler, const CG::CGTYPE &Type,

/__w/llvm/llvm/src/sycl/source/detail/queue_impl.cpp:325:24: error: lambda capture 'Context' is not used [-Werror,-Wunused-lambda-capture] 325 | [&Context, &CheckEvent](const sycl::event &Event) {

@sergey-semenov
Copy link
Contributor

@martygrant Thanks for letting me know, opened #12523

sergey-semenov added a commit that referenced this pull request Jan 29, 2024
@aelovikov-intel
Copy link
Contributor

It seems there are remaining warnings caused by this (https://github.com/intel/llvm/actions/runs/7701911776/job/20989034380):

/__w/llvm/llvm/src/sycl/unittests/scheduler/InOrderQueueHostTaskDeps.cpp:94:7: error: 'unique_lock' may not intend to support class template argument deduction [-Werror,-Wctad-maybe-unsupported]
   94 |       std::unique_lock lk(CvMutex);
      |       ^

@sergey-semenov
Copy link
Contributor

Opened #12539

aelovikov-intel pushed a commit that referenced this pull request Jan 30, 2024
Fixes remaining warnings introduced in
#11758
dm-vodopyanov pushed a commit that referenced this pull request Apr 16, 2024
Commit #11758 added extra event as
dependency to fix host task vs kernel dependencies for inorder queue.
That logic missed part when we try to bypass scheduler for kernel with
no dependencies. This commit fixes it and allows to bypass scheduler if
previous commands are enqueued (no host task is involved).

---------

Signed-off-by: Tikhomirova, Kseniya <[email protected]>
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.

8 participants