[SYCL][L0][Plugin] Add support for batching copy commands #5155

asudarsa · 2021-12-15T23:56:40Z

Signed-off-by: Arvind Sudarsanam [email protected]

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa · 2021-12-15T23:56:54Z

/summary:run

sycl/plugins/level_zero/pi_level_zero.cpp

Signed-off-by: Arvind Sudarsanam <[email protected]>

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel · 2021-12-16T20:26:54Z

sycl/plugins/level_zero/pi_level_zero.hpp

+    pi_uint32 QueueBatchSize = {0};
+  } command_batch;
+
+  command_batch ComputeCommandBatch, CopyCommandBatch;


Please add comments

Done. Thanks

smaslov-intel · 2021-12-16T20:28:27Z

sycl/plugins/level_zero/pi_level_zero.hpp

+  // be batched together.
+  // For copy commands, IsCopy is set to 'true'.
+  // For non-copy commands, IsCopy is set to 'false'.
+  bool isBatchingAllowed(bool IsCopy);


Suggested change

bool isBatchingAllowed(bool IsCopy);

bool isBatchingAllowed(bool IsCopy) const;

Done as part of bigger commit.

smaslov-intel · 2021-12-16T20:30:59Z

sycl/plugins/level_zero/pi_level_zero.hpp

+  pi_result executeOpenCommandList(bool IsCopy);
+
+  // Wrapper function to execute both OpenCommandLists (Copy and Compute).
+  pi_result executeOpenCommandLists() {


To make it more distinct from the executeOpenCommandList:

Suggested change

pi_result executeOpenCommandLists() {

pi_result executeAllOpenCommandLists() {

Also please add comment about how one should choose between the 2 versions. I.e. in what circumstances both batches need to be closed.

Done. Thanks.

smaslov-intel · 2021-12-16T20:36:20Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -2817,6 +2831,15 @@ pi_result piContextRelease(pi_context Context) {
  return ContextReleaseHelper(Context);
 }

+// This function will initialize batch sizes for the compute anc copy command
+// lists
+static void initializeQueueBatchSizes(pi_queue Queue) {


please move _pi_queue ctor implementation to pi_level_zero.cpp, and remove this function

Done. Thanks.

smaslov-intel · 2021-12-16T20:49:45Z

sycl/plugins/level_zero/pi_level_zero.cpp

+      if (Event->Queue->hasOpenCommandList(false /* IsCopy */) &&
+          Event->Queue->ComputeCommandBatch.OpenCommandList->first ==
+              Event->ZeCommandList) {
+        if (auto Res = Event->Queue->executeOpenCommandList(false /* IsCopy */))


maybe

using IsCopy = bool;

and then you can write arguments like

IsCopy{false}

Done. Thanks

smaslov-intel · 2021-12-16T20:57:06Z

sycl/plugins/level_zero/pi_level_zero.cpp

+  bool UseCopyEngine =
+      (!(Queue->isInOrderQueue()) || UseCopyEngineForInOrderQueue) &&
+      PreferCopyEngine && Queue->Device->hasCopyEngine();


Let's make it into utility pi_queue::useCopyEngine(bool PreferCopyEngine = true)

Done. Thanks

smaslov-intel · 2021-12-16T20:59:33Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -5551,7 +5590,8 @@ static pi_result enqueueMemCopyHelper(pi_command_type CommandType,
          pi_cast<std::uintptr_t>(ZeEvent));
  printZeEventList(WaitList);

-  if (auto Res = Queue->executeCommandList(CommandList, BlockingWrite))
+  if (auto Res =
+          Queue->executeCommandList(CommandList, BlockingWrite, OkToBatch))


What are the commands that we still don't batch? Maybe we should just make the last argument (OkToBatch) true by default now?

We do have considerable number of call-sites where this argument is 'false' (wait commands, membuffermap, memadvise, etc). So, we don't need to change the default now.

smaslov-intel

LGTM overall, few minor suggestions

Signed-off-by: Arvind Sudarsanam <[email protected]>

…changes Signed-off-by: Arvind Sudarsanam <[email protected]>

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa · 2021-12-17T16:00:40Z

Thanks Sergey for all your comments. I have tried my best to handle them. Please take a look.

smaslov-intel · 2021-12-17T17:34:32Z

sycl/plugins/level_zero/pi_level_zero.cpp

+}();
+
+// Static variable that holds batch config info for copy command batching.
+static const zeCommandListBatchConfig ZeCommandListBatchCopyConfig = [] {


please re-use the initialization lambda from the compute config

I mean turn lambda into a named function and us it to init both of configs

Got it....I made the appropriate changes. Thanks

smaslov-intel

LGTM, just one comment to re-use code for env var inititalization

…changes Signed-off-by: Arvind Sudarsanam <[email protected]>

Signed-off-by: Arvind Sudarsanam <[email protected]>

bader

sycl/doc/EnvironmentVariables.md looks good to me.

* upstream/sycl: (1382 commits) [SYCL][XPTI] Report memory allocation info from SYCL runtime (intel#5172) [CI] Switch labels for OCL x64 job (intel#5185) [SYCL] Add basic support for the generic_space address space (intel#5148) [CI] Update CODEOWNERS for SYCL printf support passes (intel#5199) [SYCL][Matrix] Enable wi_slice for joint_matrix (intel#4979) [SYCL][Group algorithms] Move group sort extension to experimental (intel#5169) [SYCL] Fix kernel bundles don't really carry kernel IDs (intel#5121) [SYCL] Initial printf support for non-constant AS format strings (intel#5069) [SYCL][NFC] Fix static code analysis concerns (intel#5189) [SYCL][Doc] Fix typos to fix doc build (intel#5190) [Driver][SYCL] Turn on -fsycl-dead-args-optimization by default (intel#3004) [SYCL][L0][Plugin] Add support for batching copy commands (intel#5155) [CI] Add cache checkout script to docker containers (intel#5184) [SYCL][Doc] Add HIP backend to the filter selector (intel#5176) [Doc] Add documentation for Docker images (intel#4778) [LIBCLC] Add functionality for in-kernel asserts for CUDA backend (intel#5174) Force opt to use new pass manager in exponential-deferred-inlining test after a8c2ba1 [SYCL] Add vec and marray support to known_identity type trait (intel#5163) Correctly resolve merge conflicts Update SPV_INTEL_hw_thread_queries to rev 2 ...

[SYCL][L0][Plugin] Add support for batching copy commands

afe85df

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa requested review from bader, pvchupin and smaslov-intel as code owners December 15, 2021 23:56

asudarsa mentioned this pull request Dec 16, 2021

[SYCL][Plugin][L0] Add test for batching copy commands intel/llvm-test-suite#622

Merged

keryell reviewed Dec 16, 2021

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Show resolved Hide resolved

[SYCL][L0][Plugin] Minor syntax correction

fff84ee

Signed-off-by: Arvind Sudarsanam <[email protected]>

smaslov-intel reviewed Dec 16, 2021

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Show resolved Hide resolved

smaslov-intel reviewed Dec 16, 2021

View reviewed changes

smaslov-intel suggested changes Dec 16, 2021

View reviewed changes

asudarsa added 4 commits December 17, 2021 07:40

[SYCL][Plugin][L0] Addressing review comments for copy batching changes

36b78e9

Signed-off-by: Arvind Sudarsanam <[email protected]>

[SYCL][Plugin][L0] Addressing more review comments for copy batching …

5aac2ee

…changes Signed-off-by: Arvind Sudarsanam <[email protected]>

[SYCL][Plugin][L0] Addressing more review comments for copy batching …

42cc249

…changes Signed-off-by: Arvind Sudarsanam <[email protected]>

[SYCL][Plugin][L0] Fix clang-format issues

3c2d6e0

Signed-off-by: Arvind Sudarsanam <[email protected]>

smaslov-intel reviewed Dec 17, 2021

View reviewed changes

asudarsa added 2 commits December 17, 2021 10:08

[SYCL][Plugin][L0] Addressing more review comments for copy batching …

581d69e

…changes Signed-off-by: Arvind Sudarsanam <[email protected]>

[SYCL][Plugin][L0] Modify comment

9a07a7e

Signed-off-by: Arvind Sudarsanam <[email protected]>

smaslov-intel approved these changes Dec 17, 2021

View reviewed changes

bader approved these changes Dec 20, 2021

View reviewed changes

againull merged commit 4c3e699 into intel:sycl Dec 20, 2021

	bool isBatchingAllowed(bool IsCopy);
	bool isBatchingAllowed(bool IsCopy) const;

	pi_result executeOpenCommandLists() {
	pi_result executeAllOpenCommandLists() {

[SYCL][L0][Plugin] Add support for batching copy commands #5155

[SYCL][L0][Plugin] Add support for batching copy commands #5155

Uh oh!

Conversation

asudarsa commented Dec 15, 2021

Uh oh!

asudarsa commented Dec 15, 2021

Uh oh!

Uh oh!

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!

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!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

asudarsa commented Dec 17, 2021

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!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!