[SYCL] Add support for devices white list #867

romanovvlad · 2019-11-24T16:29:57Z

Add support for SYCL_DEVICE_WHITE_LIST config which allows to specify the white
list of devices and their minimum driver version. This is done by setting
configuration in the following format:

DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}|DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}

Where values in {{ }} can be specified in ECMAScript regular expressions
pattern syntax

Devices that do not satisfy the pattern from the white list are ignored.

Signed-off-by: Vlad Romanov [email protected]

romanovvlad · 2019-11-24T16:32:24Z

sycl/source/detail/platform_impl.cpp

+};
+
+static std::vector<DevDescT> getWhiteListDesc() {
+  // TODO: Replace with const char *str =


Waiting for #859 to resolve this.

AlexeySachkov

Please add a test

sycl/source/detail/platform_impl.cpp

asavonic

Please add a test.

asavonic · 2019-11-25T05:06:23Z

sycl/source/detail/platform_impl.cpp

+      continue;
+
+    pi_devices[i] = pi_devices.back();
+    pi_devices.pop_back();


This is going to change order of devices, right? It should not matter for a properly written program, but can be unexpected. It should be easy to change the loop and iterate forward instead (with two indices: insertion point and current device).

Sorry, didn't get the idea how to change the loop.
Furthermore I don't think we should try to preserve the order as program relying on this is 200% invalid. The are source of order change already anyway(icd filenames, OCL_ICD_FILENAMES, OCL_ICD_VENDORS).

I don't think we should try to preserve the order as program relying on this is 200% invalid.

Relying on the order is invalid, but let's not break it if we can.

The idea is simple: iterate over devices (index i) and have another index where the next "good" device should be written.

int insert_idx = 0; for (int i = 0; i < devices.size(); ++i) { if (isgood(devices[i]) { devices[insert_idx++] = devices[i]; } } devices.resize(insert_idx);

Ok, will do.

sycl/source/detail/platform_impl.cpp

romanovvlad · 2019-11-25T19:49:37Z

Don't know how to write a test, do you have some ideas?
The only idea I have is to write the test which, during the first run, prints available device and form some string matching this device, for the second run the string is set as white listed device and test checks that it's really available, for the third run white list is empty and the test verifies that there is no devices available.

asavonic · 2019-11-25T20:54:09Z

Don't know how to write a test, do you have some ideas?
The only idea I have is to write the test which, during the first run, prints available device and form some string matching this device, for the second run the string is set as white listed device and test checks that it's really available, for the third run white list is empty and the test verifies that there is no devices available.

Sounds good!

bader · 2019-11-26T12:38:24Z

sycl/source/detail/platform_impl.cpp

+  // TODO: Replace with const char *str =
+  // SYCLConfig<SYCL_DEVICE_WHITE_LIST>::get();
+  const char *str = getenv("SYCL_DEVICE_WHITE_LIST");
+  if (!str)


I would like to check this condition as early as possible i.e. before calling filterWhiteList to avoid unnecessary overhead.

Added additional check before calling filterWhiteList

I would like to check this condition as early as possible i.e. before calling filterWhiteList to avoid unnecessary overhead.

This is already the first operation in filterWhiteList, which overhead you are trying to avoid?

vector creation/deletion + function calls.

sycl/source/detail/platform_impl.cpp

Add support for SYCL_DEVICE_WHITE_LIST config which allows to specify the white list of devices and their minimum driver version. This is done by setting configuration in the following format: DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}}|DeviceName:{{XXX}},DriverVersion:{{X.Y.Z.W}} Where values in {{ }} can be specified in ECMAScript regular expressions pattern syntax Devices that do not satisfy the pattern from the white list are ignored. Signed-off-by: Vlad Romanov <[email protected]>

Signed-off-by: Vlad Romanov <[email protected]>

asavonic · 2019-11-26T15:26:37Z

sycl/source/detail/platform_impl.cpp

+    // Skip ':'
+    str += 1;
+
+    if ('{' != *str || '{' != *(str + 1))


You should check that *str != '\0'.

No, it is actually not needed here.

Signed-off-by: Vlad Romanov <[email protected]>

romanovvlad · 2019-11-26T15:51:05Z

Don't know how to write a test, do you have some ideas?
The only idea I have is to write the test which, during the first run, prints available device and form some string matching this device, for the second run the string is set as white listed device and test checks that it's really available, for the third run white list is empty and the test verifies that there is no devices available.

Sounds good!

Done.

sycl/source/detail/platform_impl.cpp

asavonic

LGTM.

All comments are resolved.

keryell

This looks like a good introduction about why we need #include <algorithm> :-)
Perhaps https://www.youtube.com/watch?v=pUEnO6SvAMo might be interesting.

keryell · 2019-11-27T00:41:52Z

sycl/test/config/white_list.cpp

+        // As device name string will be used as regexp pattern, we need to
+        // get rid of symbols that can be treated in a special way.
+        // Replace common special symbols with '.' which matches to any sybmol
+        for (char &Sym : DevName) {


What about using algorithms instead?

Ok, will replace with:

std::replace_if(DevName.begin(), DevName.end(), [](const char Sym) { return '(' == Sym || ')' == Sym; }, '.');

Speaking honestly, I think loop code is more readable.

I was thinking to a simpler:

std::replace(DevName.begin(), DevName.end(), '(', '.'); std::replace(DevName.begin(), DevName.end(), ')', '.');

since everything is in the cache, it should be fast.

I understand that it's a test but I don't think we should sacrifice performance in order to make C++ code more readable...

I think it is very subjective.

for (char &Sym : DevName) { if (')' == Sym || '(' == Sym) Sym = '.'; }

IMO this code is more readable than:

std::replace(DevName.begin(), DevName.end(), '(', '.'); std::replace(DevName.begin(), DevName.end(), ')', '.');

keryell · 2019-11-27T00:42:46Z

sycl/source/detail/platform_impl.cpp

+  const char driverVerStr[] = "DriverVersion";
+  decDescs.emplace_back();
+  while ('\0' != *str) {
+    const char **valuePtr = nullptr;


It smells badly in a C++ code...

keryell · 2019-11-27T00:44:05Z

sycl/source/detail/platform_impl.cpp

+      throw sycl::runtime_error("Malformed device white list");
+
+    ++str;
+  }


Is it possible to rewrite this loop in something higher-level so it is more understandable?

romanovvlad requested review from alexbatashev, asavonic, AlexeySachkov and sergey-semenov November 24, 2019 16:29

romanovvlad assigned AlexeySachkov Nov 24, 2019

romanovvlad commented Nov 24, 2019

View reviewed changes

AlexeySachkov previously requested changes Nov 24, 2019

View reviewed changes

romanovvlad commented Nov 25, 2019

View reviewed changes

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

asavonic suggested changes Nov 25, 2019

View reviewed changes

MrSidims reviewed Nov 25, 2019

View reviewed changes

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

romanovvlad force-pushed the private/vromanov/PlatformsWhiteList2 branch from c4ddc85 to cc76e05 Compare November 25, 2019 19:33

romanovvlad force-pushed the private/vromanov/PlatformsWhiteList2 branch from cc76e05 to 8f879e6 Compare November 26, 2019 09:50

bader requested review from AlexeySachkov, asavonic and MrSidims November 26, 2019 12:26

bader reviewed Nov 26, 2019

View reviewed changes

AlexeySachkov reviewed Nov 26, 2019

View reviewed changes

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

romanovvlad added 3 commits November 26, 2019 16:44

apply comments

81f3b6e

Signed-off-by: Vlad Romanov <[email protected]>

applying comments

98067b0

Signed-off-by: Vlad Romanov <[email protected]>

romanovvlad force-pushed the private/vromanov/PlatformsWhiteList2 branch from 8f879e6 to 98067b0 Compare November 26, 2019 14:37

asavonic reviewed Nov 26, 2019

View reviewed changes

romanovvlad added 2 commits November 26, 2019 18:47

adding test.

9dd414b

Signed-off-by: Vlad Romanov <[email protected]>

removing and adding another test.

53f3de5

Signed-off-by: Vlad Romanov <[email protected]>

bader reviewed Nov 26, 2019

View reviewed changes

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

romanovvlad requested review from asavonic and AlexeySachkov November 26, 2019 16:15

asavonic approved these changes Nov 26, 2019

View reviewed changes

romanovvlad merged commit 4ad5263 into intel:sycl Nov 26, 2019

romanovvlad deleted the private/vromanov/PlatformsWhiteList2 branch November 26, 2019 17:30

keryell reviewed Nov 27, 2019

View reviewed changes

[SYCL] Add support for devices white list #867

[SYCL] Add support for devices white list #867

Uh oh!

Conversation

romanovvlad commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

romanovvlad commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asavonic commented Nov 25, 2019

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!

bader Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

romanovvlad commented Nov 26, 2019

Uh oh!

Uh oh!

asavonic left a comment

Choose a reason for hiding this comment

Uh oh!

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

romanovvlad Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanovvlad commented Nov 24, 2019 •

edited

Loading

romanovvlad commented Nov 25, 2019 •

edited

Loading

bader Nov 26, 2019 •

edited

Loading

romanovvlad Nov 29, 2019 •

edited

Loading