[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST #2483

glyons-intel · 2020-09-16T20:25:25Z

[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST

Fixed a memory issue, where memory had not been allocated properly. This caused a seg fault.
Refactored from a C-style to a C++ style.
Created a test case which tests both legal and illegal uses.
Updated the documentation.

Signed-off-by: Gail Lyons <[email protected]>

Added support for the case where multiple devices or platforms are listed in SYCL_DEVICE_ALLOWLIST. Also fixed a memory issue. Created a test case which tests both legal and illegal uses. Updated the documentation. Signed-off-by: Gail Lyons <[email protected]>

pvchupin

doc changes ok

s-kanaev

I didn't take a look at the test yet. Though, I have some comments.

sycl/source/detail/platform_impl.cpp

sycl/doc/EnvironmentVariables.md

sycl/source/detail/platform_impl.cpp

romanovvlad · 2020-09-17T09:54:32Z

[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST

Added support for the case where multiple devices or
platforms are listed in SYCL_DEVICE_ALLOWLIST.
Also fixed a memory issue.
Created a test case which tests both legal and illegal uses.
Updated the documentation.

@glyons-intel
Could you please elaborate more on "case where multiple devices or platforms are listed"? Could you please provide an example which doesn't work? Also could you please provide more details about memory issue which is being fixed?

Signed-off-by: Gail Lyons <[email protected]>

kbobrovs · 2020-09-23T09:12:55Z

sycl/doc/EnvironmentVariables.md

@@ -22,7 +22,7 @@ subject to change. Do not rely on these variables in production code.
 | SYCL_DISABLE_EXECUTION_GRAPH_CLEANUP | Any(\*) | Disable cleanup of finished command nodes at host-device synchronization points. |
 | SYCL_THROW_ON_BLOCK | Any(\*) | Throw an exception on attempt to wait for a blocked command.  |
 | SYCL_DEVICELIB_INHIBIT_NATIVE | String of device library extensions (separated by a whitespace) | Do not rely on device native support for devicelib extensions listed in this option. |
-| SYCL_DEVICE_ALLOWLIST | A list of devices and their minimum driver version following the pattern: DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}. Also may contain PlatformName and PlatformVersion | Filter out devices that do not match the pattern specified. Regular expression can be passed and the DPC++ runtime will select only those devices which satisfy the regex. |
+| SYCL_DEVICE_ALLOWLIST | A list of devices and their minimum driver version following the pattern: DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}. Also may contain PlatformName and PlatformVersion | Filter out devices that do not match the pattern specified. Regular expression can be passed and the DPC++ runtime will select only those devices which satisfy the regex. Note that the device name, platform name and platform version are regular expressions.  Special characters, such as parenthesis, must be escaped. The device driver version is treated as 4 regular expressions, separated by ".". More than one device can be specified using "|".|


The device driver version is treated as 4 regular expressions, separated by "."

does this mean '.' cannot be used in the regexp? Please add.

kbobrovs · 2020-09-23T09:14:47Z

sycl/source/detail/platform_impl.cpp

-      decDescs.emplace_back();
-    else if (',' != *str)
-      throw sycl::runtime_error("Malformed device allowlist", PI_INVALID_VALUE);
+MatchState matchVersions(std::string Version1, std::string Version2) {


NIT: does it make sense to move matching-related code to a separate source, to avoid clutter?

It does not look like this in the source. The diff makes it looks confusing. Between the pink and the green, getAllowListDesc() ends in the normal way, and there is the convertVersionString() function . Then we find matchVersions().

s-kanaev · 2020-09-23T09:56:51Z

sycl/source/detail/platform_impl.cpp

+  if (V1.size() != V2.size()) {
+    return MatchState::NOMATCH;
  }


Oh, why is that?
Unspecified version elements may be set as zero. I.e. if user specifies major version only, then we can say that minor and patch version numbers are specified as zero so we still will match 3 and 3.1.10.4.

@romanovvlad what do you think about it?

Excellent point. We could be more flexible, and only worry about matching the fields that are listed. That had not occurred to me.

@romanovvlad, what do you think?

Agreed to have an offline discussion.

Signed-off-by: Gail Lyons <[email protected]>

sycl/doc/EnvironmentVariables.md

sycl/source/detail/platform_impl.cpp

Signed-off-by: Gail Lyons <[email protected]>

sycl/test/config/select_device.cpp

s-kanaev · 2020-09-28T19:59:58Z

sycl/test/config/select_device.cpp

+      int count = 0;
+      for (const auto &plt : platform::get_platforms()) {
+        if (!plt.has(aspect::host)) {
+          for (const auto &dev : plt.get_devices()) {


We may not have a machine with multiple GPUs in our testing infrastructure. Hence, there is a need to make sure that proper scenario is still produced.

good point.

i tested this, and it works fine.

Signed-off-by: Gail Lyons <[email protected]>

s-kanaev

RT and tests LGTM

s-kanaev · 2020-09-30T15:39:55Z

@kbobrovs, @pvchupin, please, take a look at modified documentation.

s-kanaev · 2020-09-30T15:40:06Z

/summary:run

pvchupin · 2020-09-30T16:46:01Z

sycl/doc/EnvironmentVariables.md

@@ -22,7 +22,7 @@ subject to change. Do not rely on these variables in production code.
 | SYCL_DISABLE_EXECUTION_GRAPH_CLEANUP | Any(\*) | Disable cleanup of finished command nodes at host-device synchronization points. |
 | SYCL_THROW_ON_BLOCK | Any(\*) | Throw an exception on attempt to wait for a blocked command.  |
 | SYCL_DEVICELIB_INHIBIT_NATIVE | String of device library extensions (separated by a whitespace) | Do not rely on device native support for devicelib extensions listed in this option. |
-| SYCL_DEVICE_ALLOWLIST | A list of devices and their minimum driver version following the pattern: DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}. Also may contain PlatformName and PlatformVersion | Filter out devices that do not match the pattern specified. Regular expression can be passed and the DPC++ runtime will select only those devices which satisfy the regex. |
+| SYCL_DEVICE_ALLOWLIST | A list of devices and their driver version following the pattern: DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}. Also may contain PlatformName and PlatformVersion | Filter out devices that do not match the pattern specified. Regular expression can be passed and the DPC++ runtime will select only those devices which satisfy the regex. Special characters, such as parenthesis, must be escaped. More than one device can be specified using the piping symbol "|".|


Suggested change

| SYCL_DEVICE_ALLOWLIST | A list of devices and their driver version following the pattern: DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}. Also may contain PlatformName and PlatformVersion | Filter out devices that do not match the pattern specified. Regular expression can be passed and the DPC++ runtime will select only those devices which satisfy the regex. Special characters, such as parenthesis, must be escaped. More than one device can be specified using the piping symbol "|".|

| SYCL_DEVICE_ALLOWLIST | A list of devices and their driver version following the pattern: DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}. Also may contain PlatformName and PlatformVersion | Filter out devices that do not match the pattern specified. Regular expression can be passed and the DPC++ runtime will select only those devices which satisfy the regex. Special characters, such as parenthesis, must be escaped. More than one device can be specified using the piping symbol "\|".|

escape required for pipe to visualize properly

Done. Thanks for catching this.

Signed-off-by: Gail Lyons <[email protected]>

s-kanaev · 2020-10-01T06:30:03Z

/summary:run

…_wrapper * upstream/sycl: (1021 commits) [SYCL] Enable async_work_group_copy for scalar and vector bool types (intel#2582) [SYCL] Fix element type in handler::copy (intel#2590) [NFC][SYCL] Remove unnecessary if condition (intel#2585) [SYCL][NFC] Fix SYCL lit test execution on a system w/o GPU (intel#2584) [SYCL] Add error handling for non-uniform work group size case (intel#2569) [SYCL][ESIMD] Preserve undef initializer for globals in ESIMDLowerVecArg pass (intel#2555) [SYCL] Make Level-Zero events visible on the host (intel#2576) [Driver][SYCL][NFC] Add help information for -Wno-sycl-strict (intel#2570) [SYCL] Relax test to work in Win32 environment. (intel#2580) [SYCL] Emit suppressed warnings from SYCL headers (intel#2575) [SYCL][NFC] Cover more classes with ABI tests (intel#2577) [SYCL][ESIMD] Update ESIMD tests and add raw send support. (intel#2482) [SYCL] Make ESIMD on-device tests require linux,gpu,opencl. (intel#2560) [SYCL] Release commands with no dependencies after they're enqueued (intel#2492) [SYCL] Add multi-device and multi-platform support for SYCL_DEVICE_ALLOWLIST (intel#2483) [SYCL] Try to enqueue host command depencies (intel#2561) [SYCL][ESIMD][NFC] Align namespace name with the spec guidelines (intel#2573) [SYCL][NFC] Add class layout ABI tests for memory objects (intel#2559) [SYCL] Change adress space for global variables (intel#2534) [NFC][SYCL] Fix comment. (intel#2541) ...

Update a test after llvm-project commit 379628d ("[RemoveDIs] Add flag to preserve the debug info format of input IR (#87379)", 2024-04-05). Original commit: KhronosGroup/SPIRV-LLVM-Translator@cf64d9a98402d8f

glyons-intel added 2 commits September 16, 2020 12:11

SYCL_DEVICE_ALLOWLIST fix. WIP

5c30ab7

Signed-off-by: Gail Lyons <[email protected]>

glyons-intel requested review from romanovvlad and s-kanaev September 16, 2020 20:25

glyons-intel requested review from kbobrovs, pvchupin and a team as code owners September 16, 2020 20:25

glyons-intel changed the title ~~Glyons/allowlist~~ [SYCL] Additional support for SYCL_DEVICE_ALLOWLIST #2483 Sep 16, 2020

glyons-intel changed the title ~~[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST #2483~~ [SYCL] Additional support for SYCL_DEVICE_ALLOWLIST Sep 16, 2020

pvchupin previously approved these changes Sep 17, 2020

View reviewed changes

s-kanaev reviewed Sep 17, 2020

View reviewed changes

sycl/source/detail/platform_impl.cpp Outdated Show resolved Hide resolved

sycl/source/detail/platform_impl.cpp Outdated Show resolved Hide resolved

sycl/doc/EnvironmentVariables.md Outdated Show resolved Hide resolved

sycl/source/detail/platform_impl.cpp Outdated Show resolved Hide resolved

romanovvlad requested a review from AGindinson September 17, 2020 11:00

Incorporated code review comments

aff799f

Signed-off-by: Gail Lyons <[email protected]>

glyons-intel dismissed pvchupin’s stale review via aff799f September 22, 2020 19:45

kbobrovs reviewed Sep 23, 2020

View reviewed changes

s-kanaev reviewed Sep 23, 2020

View reviewed changes

glyons-intel added 3 commits September 25, 2020 07:49

Incorporating code review comments

49e41f7

Signed-off-by: Gail Lyons <[email protected]>

Mark test case as expected to fail on cuda.

267ad4f

Signed-off-by: Gail Lyons <[email protected]>

Fixed a bug in the test program.

cfb40d7

Signed-off-by: Gail Lyons <[email protected]>

s-kanaev reviewed Sep 28, 2020

View reviewed changes

sycl/doc/EnvironmentVariables.md Outdated Show resolved Hide resolved

sycl/source/detail/platform_impl.cpp Outdated Show resolved Hide resolved

Incorporating code review comments.

55b0857

Signed-off-by: Gail Lyons <[email protected]>

s-kanaev reviewed Sep 28, 2020

View reviewed changes

Incorporated code review comments

650d475

Signed-off-by: Gail Lyons <[email protected]>

s-kanaev approved these changes Sep 30, 2020

View reviewed changes

s-kanaev requested review from pvchupin and kbobrovs September 30, 2020 15:38

s-kanaev previously approved these changes Sep 30, 2020

View reviewed changes

pvchupin reviewed Sep 30, 2020

View reviewed changes

Incorporating code review comments

5fb7393

Signed-off-by: Gail Lyons <[email protected]>

glyons-intel dismissed s-kanaev’s stale review via 5fb7393 September 30, 2020 18:24

pvchupin approved these changes Sep 30, 2020

View reviewed changes

s-kanaev approved these changes Oct 1, 2020

View reviewed changes

bader merged commit dbf31c3 into intel:sycl Oct 1, 2020

glyons-intel deleted the glyons/allowlist branch October 1, 2020 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST #2483

[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST #2483

glyons-intel commented Sep 16, 2020 •

edited

Loading

pvchupin left a comment

s-kanaev left a comment

romanovvlad commented Sep 17, 2020 •

edited

Loading

kbobrovs Sep 23, 2020

kbobrovs Sep 23, 2020

glyons-intel Sep 23, 2020

s-kanaev Sep 23, 2020

glyons-intel Sep 23, 2020

romanovvlad Sep 23, 2020

s-kanaev Sep 28, 2020

glyons-intel Sep 28, 2020

glyons-intel Sep 29, 2020

s-kanaev left a comment

s-kanaev commented Sep 30, 2020

s-kanaev commented Sep 30, 2020

pvchupin Sep 30, 2020

glyons-intel Sep 30, 2020

s-kanaev commented Oct 1, 2020

[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST #2483

[SYCL] Additional support for SYCL_DEVICE_ALLOWLIST #2483

Conversation

glyons-intel commented Sep 16, 2020 • edited Loading

pvchupin left a comment

Choose a reason for hiding this comment

s-kanaev left a comment

Choose a reason for hiding this comment

romanovvlad commented Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanaev left a comment

Choose a reason for hiding this comment

s-kanaev commented Sep 30, 2020

s-kanaev commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanaev commented Oct 1, 2020

glyons-intel commented Sep 16, 2020 •

edited

Loading

romanovvlad commented Sep 17, 2020 •

edited

Loading