[WIP] [SYCL] Add SYCLConfig helper #859

romanovvlad · 2019-11-21T08:18:20Z

SYCLConfig helper provides an access to configs that is defined in
config.hpp. Such configs can be set at compile time, using environment
variable and configuration file. SYCLConfig checks all the sources and
return value which was set using an option which has highest priority.
Current priorities are:

Environment variable
Configuration file
Compiler time

If a value is not set nullptr is returned.

Currently SYCLConfig can be used from source directory only.

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

sycl/source/detail/config.cpp

sycl/include/CL/sycl/detail/config.hpp

sycl/source/detail/config.cpp

asavonic

This actually looks pretty good.

sycl/include/CL/sycl/detail/config.hpp

sycl/source/detail/config.cpp

sycl/include/CL/sycl/detail/config.def

sycl/source/detail/config.cpp

asavonic · 2019-11-25T14:35:13Z

sycl/source/detail/config.cpp

+namespace detail {
+
+#ifndef SYCL_CONFIG_FILE_NAME
+#define SYCL_CONFIG_FILE_NAME "sycl.cfg"


sycl/source/detail/config.cpp

asavonic · 2019-11-25T14:44:53Z

sycl/source/detail/config.cpp

+        File.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
+        continue;
+      }
+      File.getline(Value, sizeof(Value), '\n');


I wonder what happens on Windows. People tend add \r\n instead of \n.

Added a logic which removes \r if it exists. Cannot write proper test as we have no env vars sensitive to the additional character at the end. Did manual testing.

All this \n should be handled transparently by iostream, isn't it???

Yes, \n is the default value of 3rd argument.. Will remove

asavonic · 2019-11-25T14:46:59Z

sycl/test/config/config.cpp

+// RUN: ls | grep dot
+// RUN: rm *.dot
+// RUN: env SYCL_PRINT_EXECUTION_GRAPH=always %t.out
+// RUN: ls | grep dot


Are you sure that this works on Windows if you don't have GNU coreutils installed?

I've reused this idea from clang LIT tests so I expect it works on the same configurations as clang LIT tests work.

SYCLConfig helper provides an access to configs that is defined in config.hpp. Such configs can be set at compile time, using environment variable and configuration file. SYCLConfig checks all the sources and return value which was set using an option which has highest priority. Current priorities are: 1. Environment variable 2. Configuration file 3. Compiler time If a value is not set nullptr is returned. Signed-off-by: Vlad Romanov <[email protected]>

side effects of env var. Signed-off-by: Vlad Romanov <[email protected]>

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

asavonic

LGTM overall. Please check what happened with strncpy.

sycl/source/detail/config.cpp

asavonic · 2019-11-25T20:59:41Z

sycl/source/detail/config.cpp

+        File.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
+        continue;
+      }
+      File.getline(Value, sizeof(Value), '\n');


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

alexbatashev

LGTM. My comments are minor and can be ignored for now.

sycl/source/detail/config.cpp

alexbatashev · 2019-11-26T05:38:11Z

sycl/source/detail/config.def

@@ -0,0 +1,14 @@
+// Singnature:


Suggested change

// Singnature:

// Syntax:

Nit. Does this sound better?

vladimirlaz · 2019-11-26T06:22:50Z

sycl/source/detail/scheduler/graph_builder.cpp

@@ -46,7 +47,7 @@ static bool IsSuitableSubReq(const Requirement *Req) {
 }

 Scheduler::GraphBuilder::GraphBuilder() {
-  if (const char *EnvVarCStr = std::getenv("SYCL_PRINT_EXECUTION_GRAPH")) {
+  if (const char *EnvVarCStr = SYCLConfig<SYCL_PRINT_EXECUTION_GRAPH>::get()) {


Can we use it for all environment variable (SYCL_DEVICE_TYPE, SYCL_DUMP_IMAGES, etc)?

We can use it for environment variables that are "getenv'ed" inside source directory.

I suggest to mention this limitation in the commit message.

Ok, will do during merge.

updated description with new sentence.

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

asavonic

LGTM.

asavonic · 2019-11-26T11:12:35Z

sycl/source/detail/scheduler/graph_builder.cpp

@@ -46,7 +47,7 @@ static bool IsSuitableSubReq(const Requirement *Req) {
 }

 Scheduler::GraphBuilder::GraphBuilder() {
-  if (const char *EnvVarCStr = std::getenv("SYCL_PRINT_EXECUTION_GRAPH")) {
+  if (const char *EnvVarCStr = SYCLConfig<SYCL_PRINT_EXECUTION_GRAPH>::get()) {


I suggest to mention this limitation in the commit message.

bader · 2019-11-26T13:31:11Z

@romanovvlad, this pull request has [WIP] in the title, so it's not ready for merge yet.
Did you merge it by mistake or just forgot to remove [WIP] tag?

romanovvlad · 2019-11-26T14:40:31Z

@romanovvlad, this pull request has [WIP] in the title, so it's not ready for merge yet.
Did you merge it by mistake or just forgot to remove [WIP] tag?

Forgot to remove [WIP] tag.

keryell

Bad news: C is a dangerous language. :-(
Good news: SYCL is programmed in C++! :-)

keryell · 2019-11-26T23:38:47Z

sycl/source/detail/config.cpp

+#include "detail/config.def"
+#undef CONFIG
+
+#define MAX_CONFIG_NAME 256


All this smells a lot old C...

Will replace with constexpr.

keryell · 2019-11-26T23:41:01Z

sycl/source/detail/config.cpp

+static void initValue(const char *Key, const char *Value) {
+#define CONFIG(Name, MaxSize, CompileTimeDef)                                  \
+  if (0 == strncmp(Key, SYCLConfigBase<Name>::MConfigName, MAX_CONFIG_NAME)) { \
+    strncpy(SYCLConfigBase<Name>::MStorage, Value, MaxSize);                   \


And what happens when you reach the limit ? A non-\0 ending string... :-(

You are right. Will fix by guaranteeing \0 at the and of the Value.

I would prefer just using safer C++ instead of lower-level C at the first place...

keryell · 2019-11-26T23:42:09Z

sycl/source/detail/config.cpp

+        File.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
+        continue;
+      }
+      File.getline(Value, sizeof(Value), '\n');


All this \n should be handled transparently by iostream, isn't it???

keryell · 2019-11-26T23:44:36Z

sycl/source/detail/config.cpp

+      }
+
+      // Handle '\r' by nullifying it
+      const std::streamsize ReadSybmols = File.gcount();


What is a Sybmols ?

Will rename to "characters".

keryell · 2019-11-26T23:50:21Z

sycl/source/detail/config.cpp

+    }
+  }
+  Initialized = true;
+}


All this is super hackish and obfuscated compared to the obvious algorithm: iterating on a plain getline() and using std::find or another algorithm to tokenize around =

keryell · 2019-11-26T23:51:34Z

sycl/source/detail/config.hpp

+
+// Consider strings starting with __ as unset
+constexpr const char *getStrOrNullptr(const char *Str) {
+  return (Str[0] == '_' && Str[1] == '_') ? nullptr : Str;


What happened if a string has less than 2 characters?

Will add check for that.

romanovvlad requested review from alexbatashev and asavonic November 21, 2019 08:18

romanovvlad assigned alexbatashev Nov 21, 2019

romanovvlad commented Nov 21, 2019

View reviewed changes

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

romanovvlad commented Nov 21, 2019

View reviewed changes

sycl/include/CL/sycl/detail/config.hpp Outdated Show resolved Hide resolved

romanovvlad commented Nov 21, 2019

View reviewed changes

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

asavonic reviewed Nov 21, 2019

View reviewed changes

sycl/include/CL/sycl/detail/config.hpp Outdated Show resolved Hide resolved

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

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

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

alexbatashev reviewed Nov 21, 2019

View reviewed changes

sycl/include/CL/sycl/detail/config.def Outdated Show resolved Hide resolved

alexbatashev reviewed Nov 21, 2019

View reviewed changes

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

alexbatashev reviewed Nov 21, 2019

View reviewed changes

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

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

romanovvlad force-pushed the private/vromanov/AddingConfigReader branch from c4ba968 to 32bbc2b Compare November 21, 2019 18:08

romanovvlad changed the title ~~[SYCL] Add SYCLConfig helper~~ [WIP] [SYCL] Add SYCLConfig helper Nov 21, 2019

romanovvlad force-pushed the private/vromanov/AddingConfigReader branch from 32bbc2b to 41b5fc1 Compare November 22, 2019 15:38

romanovvlad mentioned this pull request Nov 24, 2019

[SYCL] Add support for devices white list #867

Merged

asavonic reviewed Nov 25, 2019

View reviewed changes

asavonic suggested changes Nov 25, 2019

View reviewed changes

romanovvlad added 4 commits November 25, 2019 21:14

applied comments, moved to source directory and update test to check

56a5e5a

side effects of env var. Signed-off-by: Vlad Romanov <[email protected]>

fixed include quotes

0304541

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

adding missing source files

d8ec32d

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

romanovvlad force-pushed the private/vromanov/AddingConfigReader branch from 1c726ff to 48c43e8 Compare November 25, 2019 18:26

applying comments

4c1f4d1

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

romanovvlad force-pushed the private/vromanov/AddingConfigReader branch from 48c43e8 to 4c1f4d1 Compare November 25, 2019 19:41

asavonic reviewed Nov 25, 2019

View reviewed changes

apply comments

6897706

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

alexbatashev previously approved these changes Nov 26, 2019

View reviewed changes

vladimirlaz reviewed Nov 26, 2019

View reviewed changes

apply comments

8f5e519

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

romanovvlad dismissed alexbatashev’s stale review via 8f5e519 November 26, 2019 10:10

asavonic approved these changes Nov 26, 2019

View reviewed changes

romanovvlad merged commit edb985a into intel:sycl Nov 26, 2019

romanovvlad deleted the private/vromanov/AddingConfigReader branch November 26, 2019 13:01

keryell reviewed Nov 26, 2019

View reviewed changes

[WIP] [SYCL] Add SYCLConfig helper #859

[WIP] [SYCL] Add SYCLConfig helper #859

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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!

asavonic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexbatashev left a comment

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!

asavonic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented Nov 26, 2019

Uh oh!

romanovvlad commented Nov 26, 2019

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!

romanovvlad commented Nov 21, 2019 •

edited

Loading