[SYCL][L0][PI] Modify SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE environment variable to allow user a finer control over copy engines #4333

asudarsa · 2021-08-13T19:43:12Z

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

…ontrol over copy engines Signed-off-by: Arvind Sudarsanam <[email protected]>

smaslov-intel · 2021-08-13T22:46:18Z

sycl/doc/EnvironmentVariables.md

 | `SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE_FOR_D2D_COPY` (experimental) | Integer | Allows the use of copy engine, if available in the device, in Level Zero plugin for device to device copy operations. The default is 0. This option is experimental and will be removed once heuristics are added to make a decision about use of copy engine for device to device copy operations. |
+| `SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE_RANGE` | Any(\*) | This environment variable enables users to select a desired range of copy engines to use for copy operations. The value of this environment variable is a pair of the form "lower_index:upper_index" where the indices point to copy engines in a list of all available copy engines.


Can't we just extend existing SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE to [optionally] take a range now? The current 0/1 values would continue mean disable/enable all. Explain the lower/upper bounds of the range. Also since the size of the list is not known to the user it is good to have "1" to mean all.

I updated this in the latest commit. Can you please check it and see if it's missing something? Thanks.

smaslov-intel · 2021-08-16T22:27:35Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -60,12 +60,28 @@ static const bool UseCopyEngineForD2DCopy = [] {
  return (CopyEngineForD2DCopy && (std::stoi(CopyEngineForD2DCopy) != 0));
 }();

+// TODO: Add support for non-zero values of SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE


But you already support 1-digit non-zero values below. Comment outdated? Also why need this check for exactly 1 symbol?

Removed the unnecessary check and the comment. Thanks

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel · 2021-08-16T22:37:11Z

sycl/plugins/level_zero/pi_level_zero.cpp

+    if (CopyEngineRange[0] == '0')
+      return std::pair<int, int>(-1, -1);
+    else
+      return std::pair<int, int>(0, 8);


Can we avoid hardcoding 8 here, e.g. have -1 mean no upper bound

I replaced this with INT_MAX. That's the right value to use as we want to use ALL copy engines here.

smaslov-intel · 2021-08-16T22:38:34Z

sycl/plugins/level_zero/pi_level_zero.cpp

+      return std::pair<int, int>(0, 8);
+  }
+  int LowerCopyEngineIndex = (CopyEngineRange) ? (CopyEngineRange[0] - '0') : 0;
+  int UpperCopyEngineIndex = (CopyEngineRange) ? (CopyEngineRange[2] - '0') : 8;


the access CopyEngineRange[2] can be past the allocated string, please check
also check full syntax "a:b"

I modified the logic here. Please check.

smaslov-intel · 2021-08-16T22:47:05Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -1056,6 +1072,11 @@ _pi_queue::getZeCopyCommandQueue(int *CopyQueueIndex,
                                 int *CopyQueueGroupIndex) {
  assert(CopyQueueIndex);
  int n = ZeCopyCommandQueues.size();
+  int LowerCopyQueueIndex = getRangeOfAllowedCopyEngines.first;
+  int UpperCopyQueueIndex = getRangeOfAllowedCopyEngines.second;
+  LowerCopyQueueIndex = std::max(0, LowerCopyQueueIndex);


This will compute to "0" if LowerCopyQueueIndex == -1, why is this OK?

I added a check to see if LowerCopyQueueIndex is greater than UpperCopyQueueIndex and return nullptr if that's the case.

This is an additional subtle check on top of this questionable computation. Why don't you just check for -1 range and return -1?

Will address this in next patch. Thanks.

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel · 2021-08-16T22:54:08Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -2572,24 +2596,34 @@ pi_result piQueueCreate(pi_context Context, pi_device Device,
           &ZeCommandQueueDesc, // TODO: translate properties
           &ZeComputeCommandQueue));

+  // Create additional queues to link copy engines and push them into


Move the comment to line 2621

Reordered comments.

smaslov-intel

Please address comments

smaslov-intel · 2021-08-17T04:59:43Z

sycl/plugins/level_zero/pi_level_zero.cpp

+// available copy engines can be used.
+static const std::pair<int, int> getRangeOfAllowedCopyEngines = [] {
+  std::string CopyEngineRange =
+      std::getenv("SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE");


I think you cannot create a string from a null pointer, so have to check that case.

That's right. That could be the reason for the failures I am seeing as well. Will correct this.

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel

Please address more comments

smaslov-intel · 2021-08-17T05:34:36Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -1089,7 +1089,8 @@ _pi_queue::getZeCopyCommandQueue(int *CopyQueueIndex,
  int n = ZeCopyCommandQueues.size();
  int LowerCopyQueueIndex = getRangeOfAllowedCopyEngines.first;
  int UpperCopyQueueIndex = getRangeOfAllowedCopyEngines.second;
-  LowerCopyQueueIndex = std::max(0, LowerCopyQueueIndex);
+  LowerCopyQueueIndex =
+      (LowerCopyQueueIndex == -1) ? -1 : std::max(0, LowerCopyQueueIndex);


if your range is [-1:-1] then the check at line 1098 is not going to evaluate to "true"

sycl/plugins/level_zero/pi_level_zero.cpp

bader

Please, add tests for this code.

bader · 2021-08-17T06:34:17Z

sycl/plugins/level_zero/pi_level_zero.cpp

+    else
+      return std::pair<int, int>(-1, -1); // No copy engines will be used.


Suggested change

else

return std::pair<int, int>(-1, -1); // No copy engines will be used.

return std::pair<int, int>(-1, -1); // No copy engines will be used.

bader · 2021-08-17T06:35:12Z

sycl/plugins/level_zero/pi_level_zero.cpp

+  if (!EnvVar) {
+    return std::pair<int, int>(0, INT_MAX);
+  }


Suggested change

if (!EnvVar) {

return std::pair<int, int>(0, INT_MAX);

}

if (!EnvVar)

return std::pair<int, int>(0, INT_MAX);

bader · 2021-08-17T06:41:54Z

sycl/plugins/level_zero/pi_level_zero.cpp

+  int LowerCopyEngineIndex = std::stoi(CopyEngineRange.substr(0, pos));
+  int UpperCopyEngineIndex = std::stoi(CopyEngineRange.substr(pos + 1));
+  return std::pair<int, int>(LowerCopyEngineIndex, UpperCopyEngineIndex);


Looking at surrounding code it seems like there must be additional check for invalid values.
From my understanding, the requirement is: -1 <= LowerCopyEngineIndex <= UpperCopyEngineIndex <= INT_MAX.

<= INT_MAX is validated by std::stoi, but we must explicitly check other limitations.

LowerCopyEngineIndex <= UpperCopyEngineIndex is also tested at one place, although I would check it right here.

-1 <= LowerCopyEngineIndex - is not checked

Addressed in next patch. Thanks

asudarsa · 2021-08-17T16:47:15Z

The test failing in linux build is passing locally. I have restarted the test. I see that this test is failing for other PRs as well.

[SYCL][L0][PI] Add a new environment variable to allow user a finer c…

b372aef

…ontrol over copy engines Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa requested review from bader, pvchupin and smaslov-intel as code owners August 13, 2021 19:43

asudarsa added 2 commits August 13, 2021 12:51

Fix issue with copy engine group index selection

08c43d8

Fix clang-format issue

57d3c21

smaslov-intel suggested changes Aug 13, 2021

View reviewed changes

Merging two env vars

3484fcb

smaslov-intel reviewed Aug 16, 2021

View reviewed changes

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

smaslov-intel reviewed Aug 16, 2021

View reviewed changes

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

smaslov-intel reviewed Aug 16, 2021

View reviewed changes

smaslov-intel suggested changes Aug 16, 2021

View reviewed changes

Address review comments

119f8e2

asudarsa changed the title ~~[SYCL][L0][PI] Add a new environment variable to allow user a finer control over copy engines~~ [SYCL][L0][PI] Modify SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE environment variable to allow user a finer control over copy engines Aug 17, 2021

smaslov-intel reviewed Aug 17, 2021

View reviewed changes

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

smaslov-intel suggested changes Aug 17, 2021

View reviewed changes

asudarsa added 2 commits August 16, 2021 22:26

Address more review comments

6a44253

Address more review comments

b7dd092

smaslov-intel reviewed Aug 17, 2021

View reviewed changes

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

Address more review comments

895946d

smaslov-intel previously approved these changes Aug 17, 2021

View reviewed changes

bader requested changes Aug 17, 2021

View reviewed changes

Address more review comments

e2adcfd

asudarsa dismissed smaslov-intel’s stale review via e2adcfd August 17, 2021 13:48

smaslov-intel approved these changes Aug 17, 2021

View reviewed changes

bader approved these changes Aug 18, 2021

View reviewed changes

bader merged commit 3828df5 into sycl Aug 18, 2021

bader deleted the add_new_link_engine_option branch August 18, 2021 07:35

		\| `SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE_FOR_D2D_COPY` (experimental) \| Integer \| Allows the use of copy engine, if available in the device, in Level Zero plugin for device to device copy operations. The default is 0. This option is experimental and will be removed once heuristics are added to make a decision about use of copy engine for device to device copy operations. \|
		\| `SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE_RANGE` \| Any(\*) \| This environment variable enables users to select a desired range of copy engines to use for copy operations. The value of this environment variable is a pair of the form "lower_index:upper_index" where the indices point to copy engines in a list of all available copy engines.

		else
		return std::pair<int, int>(-1, -1); // No copy engines will be used.

[SYCL][L0][PI] Modify SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE environment variable to allow user a finer control over copy engines #4333

[SYCL][L0][PI] Modify SYCL_PI_LEVEL_ZERO_USE_COPY_ENGINE environment variable to allow user a finer control over copy engines #4333

Uh oh!

Conversation

asudarsa commented Aug 13, 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!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader 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!

asudarsa commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

asudarsa commented Aug 17, 2021 •

edited

Loading