[SYCL] Make host task blocking and detach empty command. Part 1 #6995

KseniyaTikhomirova · 2022-10-07T23:17:35Z

It is intended to solve existing gap in implementation that kernel enqueue depending on host task via depends_on will wait for host task completion in its own enqueue. I want to change this approach and make it similar with the one we use when we have memory dependencies.
It also should help with cleanup process because now host task is represented with two instances - ExecCGCommand and EmptyCommand that introduces leaks in some scenarios.

Signed-off-by: Tikhomirova, Kseniya [email protected]

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

hdelan · 2022-10-14T14:04:52Z

Could you give a brief description of what this PR is achieving and why it is needed? Thanks

KseniyaTikhomirova · 2022-10-14T14:14:57Z

Could you give a brief description of what this PR is achieving and why it is needed? Thanks

It is intended to solve existing gap in implementation that kernel enqueue depending on host task via depends_on will wait for host task completion in its own enqueue. I want to change this approach and make it similar with the one we use when we have memory dependencies.
It also should help with cleanup process because now host task is represented with two instances - ExecCGCommand and EmptyCommand that introduces leaks in some scenarios.

TO conclude this PR (not fully ready yet) has intension to:

make command enqueue to not wait for host task completion in its enqueueImp and to be enqueued on host task completion -> holding read lock less, give a chance to the commands with different dependency stack to be enqueued earlier
fix some memory leaks related to pair host task + empty task
ideally to remove empty task at all as class

Please let me know if you need more details or have any concerns.

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

KseniyaTikhomirova · 2022-11-07T18:14:18Z

fixes #7146

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

sergey-semenov

LGTM aside from some minor comments, feel free to apply separately.

sergey-semenov · 2022-12-12T14:43:50Z

sycl/source/detail/scheduler/commands.hpp

-    return MEnqueueStatus == EnqueueResultT::SyclEnqueueBlocked;
+    return MIsBlockable && MEnqueueStatus == EnqueueResultT::SyclEnqueueBlocked;
+  }
+  // Shows thst command could be enqueud, but is blocking enqueue of all


Suggested change

// Shows thst command could be enqueud, but is blocking enqueue of all

// Shows that command could be enqueued, but is blocking enqueue of all

fixed in e213e81

sergey-semenov · 2022-12-12T15:12:23Z

sycl/source/detail/scheduler/commands.hpp

@@ -325,6 +337,14 @@ class Command {
  /// Indicates that the node will be freed by cleanup after enqueue. Such nodes
  /// should be ignored by other cleanup mechanisms.
  bool MPostEnqueueCleanup = false;
+
+  /// Contains list of commands that depends on the host command explicitly (by
+  /// depends_on). Not involved into cleanup process since it is one-way link


Suggested change

/// depends_on). Not involved into cleanup process since it is one-way link

/// depends_on). Not involved in the cleanup process since it is one-way link

fixed in e213e81

sergey-semenov · 2022-12-12T15:12:53Z

sycl/source/detail/scheduler/commands.hpp

+
+  /// Contains list of commands that depends on the host command explicitly (by
+  /// depends_on). Not involved into cleanup process since it is one-way link
+  /// and not holds resources.


Suggested change

/// and not holds resources.

/// and does not hold resources.

fixed in e213e81

sergey-semenov · 2022-12-12T15:33:46Z

sycl/source/detail/scheduler/scheduler.hpp

                               BlockingT Blocking = NON_BLOCKING);
+
+    static bool handleBlockingCmd(Command *Cmd, EnqueueResultT &EnqueueResult,


Could you add a comment for this function?

fixed in e213e81

sycl/source/detail/scheduler/scheduler.hpp

sycl/unittests/scheduler/EnqueueWithDependsOnDeps.cpp

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

KseniyaTikhomirova · 2022-12-13T17:26:10Z

@intel/llvm-gatekeepers the change is ready for merge, sorry, closed PR by accident and should wait for checks again

pvchupin · 2022-12-13T22:37:28Z

@KseniyaTikhomirova, post-commit issue: https://github.com/intel/llvm/actions/runs/3689466995/jobs/6245440471
@aelovikov-intel is looking into it.

aelovikov-intel · 2022-12-13T22:41:51Z

#7770, hopefully.

KseniyaTikhomirova added 7 commits October 7, 2022 16:17

[SYCL] Make host task blocking and detach empty command. Part 1

a2acac8

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

Merge branch 'sycl' into empty_task_removal

1fafe3b

Temporary disable unit test for depends_on

521a4cc

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

Fix impl gaps and update/enable 1 unit test

01898c2

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

Fix & enable unit tests

007fbb7

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

Fix blocking enqueueCommand call & update queue unit tests

d442362

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

Fix build

9c05886

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

KseniyaTikhomirova added 3 commits October 21, 2022 07:03

Remove duplicated enqueue

30d46b5

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

Fix data races for BlockedUsers

65e97c9

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

Merge branch 'sycl' into empty_task_removal

65451cb

KseniyaTikhomirova mentioned this pull request Nov 2, 2022

SYCL runtime incorrectly waits on other tasks during command submission #7146

Closed

Fix event isCompleted impl

4871246

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

KseniyaTikhomirova force-pushed the empty_task_removal branch from 42b81d5 to e6b91da Compare November 16, 2022 15:40

KseniyaTikhomirova added 2 commits November 16, 2022 07:41

Merge branch 'sycl' into empty_task_removal

e6b91da

Merge branch 'sycl' into empty_task_removal

4847a74

KseniyaTikhomirova marked this pull request as ready for review December 11, 2022 19:42

KseniyaTikhomirova requested a review from a team as a code owner December 11, 2022 19:42

KseniyaTikhomirova requested review from sergey-semenov and romanovvlad December 11, 2022 19:42

sergey-semenov approved these changes Dec 12, 2022

View reviewed changes

Code-review: update comments

e213e81

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

KseniyaTikhomirova closed this Dec 13, 2022

KseniyaTikhomirova reopened this Dec 13, 2022

againull merged commit c44050a into intel:sycl Dec 13, 2022

bader mentioned this pull request Dec 15, 2022

Unit test failure after https://github.com/intel/llvm/pull/7778. #7788

Closed

	// Shows thst command could be enqueud, but is blocking enqueue of all
	// Shows that command could be enqueued, but is blocking enqueue of all

	/// depends_on). Not involved into cleanup process since it is one-way link
	/// depends_on). Not involved in the cleanup process since it is one-way link

	/// and not holds resources.
	/// and does not hold resources.

		BlockingT Blocking = NON_BLOCKING);

		static bool handleBlockingCmd(Command *Cmd, EnqueueResultT &EnqueueResult,

[SYCL] Make host task blocking and detach empty command. Part 1 #6995

[SYCL] Make host task blocking and detach empty command. Part 1 #6995

Uh oh!

Conversation

KseniyaTikhomirova commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdelan commented Oct 14, 2022

Uh oh!

KseniyaTikhomirova commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KseniyaTikhomirova commented Nov 7, 2022

Uh oh!

sergey-semenov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KseniyaTikhomirova commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvchupin commented Dec 13, 2022

Uh oh!

aelovikov-intel commented Dec 13, 2022

Uh oh!

Uh oh!

KseniyaTikhomirova commented Oct 7, 2022 •

edited

Loading

KseniyaTikhomirova commented Oct 14, 2022 •

edited

Loading

KseniyaTikhomirova commented Dec 13, 2022 •

edited

Loading