[SYCL] Support connection with multiple plugins #1490

againull · 2020-04-07T23:59:54Z

This commit enables including multiple devices of the same device_type
and changed the logic of device selection to just prefer a SYCL_BE
device if present. If someone uses SYCL_BE but appropriate device is
not present, we will simply use any other device.

Signed-off-by: Artur Gainullin artur.gainullin@intel.com

sycl/source/detail/pi.cpp

sycl/include/CL/sycl/detail/pi.hpp

sycl/source/device_selector.cpp

smaslov-intel

LGTM, just few minor comments

romanovvlad · 2020-04-08T10:02:19Z

sycl/source/detail/pi.cpp

+// are processed only once, like reading a string from environment
+// and converting it into a typed object.
+//
+template <typename T, const char *E> class Config {


Suggest reusing SYCLConfig from source/detail/config.hpp instead.

Using SYCLConfig in this case is the same as just using getenv. Introduced Config class is a singleton and was created to read environment variables only once (i.e. for efficiency). But getPreferredBE is called only once in device_selector and tracing is debugging feature, so this kind of over-complication doesn't make sense. I removed singleton, so that we don't need any static variables and global variables. I think it is ok just to read environment variables in this case.

Using SYCLConfig in this case is the same as just using getenv. Introduced Config class is a singleton and was created to read environment variables only once (i.e. for efficiency).

SYCLConfig does the same - it reads only once.

But getPreferredBE is called only once in device_selector and tracing is debugging feature, so this kind of over-complication doesn't make sense.
I believe for tracing it is called each time we call PI API. So, reading env var each time can be costly.

@romanovvlad Sorry, but looking to the code I don't understand why. When we call get() we always call getRawValue() which calls getenv().

template <ConfigID Config> class SYCLConfig { using BaseT = SYCLConfigBase<Config>; public: static const char *get() { const char *ValStr = getRawValue(); return ValStr; } private: static const char *getRawValue() { if (ConfigFromEnvEnabled) if (const char *ValStr = getenv(BaseT::MConfigName)) return ValStr;

romanovvlad · 2020-04-08T10:05:09Z

sycl/source/detail/pi.cpp

+  m_Data = (Env ? std::atoi(Env) : 0);
+}
+
+static const std::map<std::string, Backend> SyclBeMap{


Can we convert it into a pod datatype to avoid having complex global objects?

romanovvlad · 2020-04-08T10:07:56Z

sycl/source/detail/pi.cpp

-  if (PluginNames.empty() && EnableTrace)
-    std::cerr << "No Plugins Found." << std::endl;
+  if (PluginNames.empty() && trace())
+    std::cerr << "SYCL_PI_TRACE[-1]: No Plugins Found." << std::endl;


What [-1] stands for?

This says what trace-level triggered this trace line. It is useful to grep the full trace for only specific levels.

It is not clear to me why do you need to grep for specific trace levels if you can just enable only particular one instead

Moreover, -1 stands for ALL, so, it doesn't really provide information about particular trace-level

@AlexeySachkov Do you think we should not print labels like "SYCL_PI_TRACE[-1]:" at all?

@againull, I think so. From my point of view, it is better to put each message into particular category to be able enable/disable it

The "SYCL_PI_TRACE" (without the mask) prefix is useful to separate out program output from the SYCL RT traces. The mask is useful to find a concise group of messages if provided a large full trace from outside. The mask also tells what mask should be used if you are just interested in this particular group of messages.

Can we print PI_TRACE_ALL instead of hard coding "-1"? Or event have a helper which checks if some level of printing is enabled and adds it to the beginning of the printed message. Asking because with current implementation this implicit alignment between if statement and content of print can easily go out of sync.

yes, we can improve this further (probably in subsequent PRs), but printing -1 is probably better that PI_TARCE_ALL, because -1 is what users specify on command line.

sycl/source/detail/pi.cpp

sycl/source/detail/program_manager/program_manager.cpp

sycl/source/device_selector.cpp

Ruyk · 2020-04-10T08:00:11Z

sycl/source/detail/pi.cpp

+  PluginNames.push_back(std::make_pair<std::string, Backend>(
+      OPENCL_PLUGIN_NAME, SYCL_BE_PI_OPENCL));
+  PluginNames.push_back(
+      std::make_pair<std::string, Backend>(CUDA_PLUGIN_NAME, SYCL_BE_PI_CUDA));


Looks like this will force the OpenCL NVIDIA platform to come before the CUDA backend...

That's correct, by default it would be OpenCL. The explicit SYCL_BE=PI_CUDA would change that to CUDA.

Let's please move this preference inside the device_selector::select_device(). It is too subtle to have it like here. In select_device() you'd just choose the device of the preferred BE if multiple BE get the same score.

Ruyk · 2020-04-10T08:09:35Z

sycl/source/device_selector.cpp

+      return true;
+    }
+  } else if (PreferredBE == RT::Backend::SYCL_BE_PI_CUDA) {
+    if (PlatformVersion.find("CUDA") != std::string::npos) {


The PlatformVersion value for NVIDIA OpenCL also contains the string CUDA, so this will pick up OpenCL NVIDIA first. See output of NVIDIA OpenCL platform from clinfo:

Patform Vendor NVIDIA Corporation Platform Version OpenCL 1.2 CUDA 10.1.236

The solution is to change the string of the PI CUDA backend to something more specific that we can search or alternatively find CUDA string but ensure the OpenCL one is not there.

actually the useBackend or now preferredBackend isn't used anywhere else than the device_selector with this change. Do we really need that runtime code in pi.hpp, or will it be simpler to evaluate the ENV in device selector? especially as preferredBackend will always return OpenCl when no env is set :)

I now think we should just get the BE from the device's plugin, instead of querying their names.

auto BE = detail::getSyclObjImpl(dev)->getPlugin().getBackend();

Then the isDeviceOfPreferredSyclBe can be removed.

smaslov-intel

Please fix a few pointed points

hiaselhans · 2020-04-13T12:50:58Z

sycl/source/detail/pi.cpp

+static constexpr char SYCL_PI_TRACE[] = "SYCL_PI_TRACE";
+
+static Backend getBE(const char *EnvVar) {
+  const char *BE = std::getenv(SYCL_BE);


isn't this supposed to be something like std::getenv(*EnvVar)?

Yes, my mistake, fixed.

keryell · 2020-04-13T23:01:02Z

sycl/source/detail/pi.cpp

+// Lists valid environment variables.
+static constexpr char SYCL_BE[] = "SYCL_BE";
+static constexpr char SYCL_INTEROP_BE[] = "SYCL_INTEROP_BE";
+static constexpr char SYCL_PI_TRACE[] = "SYCL_PI_TRACE";


Suggested change

static constexpr char SYCL_PI_TRACE[] = "SYCL_PI_TRACE";

static constexpr auto SYCL_PI_TRACE = "SYCL_PI_TRACE";

seems simpler

keryell · 2020-04-13T23:07:56Z

sycl/source/detail/pi.cpp

    }
-    Plugins.push_back(plugin(PluginInformation));
+    Plugins.push_back(plugin(PluginInformation, PluginNames[I].second));


Perhaps an .emplace_back could work here?

keryell · 2020-04-13T23:10:45Z

sycl/source/device_selector.cpp

-  for (const auto &dev : devices)
-    if (score < operator()(dev)) {
+  for (const auto &dev : devices) {
+    int dev_score = operator()(dev);


Suggested change

int dev_score = operator()(dev);

int dev_score = (*this)(dev);

looks clearer to me

keryell · 2020-04-13T23:13:59Z

sycl/source/device_selector.cpp

      res = &dev;
-      score = operator()(dev);
+      score = dev_score;


This reminds me that we have never said anything about the fact that the device selector should always return the same value for the same device... ;-) At least calling it once and caching the result like here is safer.

Well, at some point that was intentional :-)

It is legal probably and might still be intentional. For example imagine you want to implement the random_selector. :-)

At least we want the selector to be called only once per device by the SYCL runtime inside 1 SYCL consumption or some havoc can happen...

sycl/source/detail/pi.cpp

sycl/source/device_selector.cpp

romanovvlad · 2020-04-15T14:03:59Z

sycl/source/detail/pi.cpp

+// are processed only once, like reading a string from environment
+// and converting it into a typed object.
+//
+template <typename T, const char *E> class Config {


Using SYCLConfig in this case is the same as just using getenv. Introduced Config class is a singleton and was created to read environment variables only once (i.e. for efficiency).

SYCLConfig does the same - it reads only once.

But getPreferredBE is called only once in device_selector and tracing is debugging feature, so this kind of over-complication doesn't make sense.
I believe for tracing it is called each time we call PI API. So, reading env var each time can be costly.

romanovvlad · 2020-04-15T14:07:29Z

sycl/source/detail/pi.cpp

-  if (PluginNames.empty() && EnableTrace)
-    std::cerr << "No Plugins Found." << std::endl;
+  if (PluginNames.empty() && trace())
+    std::cerr << "SYCL_PI_TRACE[-1]: No Plugins Found." << std::endl;


Can we print PI_TRACE_ALL instead of hard coding "-1"? Or event have a helper which checks if some level of printing is enabled and adds it to the beginning of the printed message. Asking because with current implementation this implicit alignment between if statement and content of print can easily go out of sync.

sycl/source/detail/pi.cpp

sycl/source/device_selector.cpp

sycl/doc/EnvironmentVariables.md

sycl/source/detail/pi.cpp

sycl/source/device.cpp

smaslov-intel

LGTM, thank you!

bader · 2020-04-22T16:03:53Z

@againull, please, fix tests on Linux and Windows.

This commit enables including multiple devices of the same device_type and changed the logic of device selection to just prefer a SYCL_BE device if present. If someone uses SYCL_BE but appropriate device is not present, we will simply use any other device. Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

* Specialize SYCLConfig template class for SYCL_BE and SYCL_PI_TRACE. * Reuse SYCLConfig instead of introducing new Config class in PI. * Use recently introduced backend enum instead of pi::Backend enum. * Print label instead of number during PI_TRACE. * Introduce helper that returns label depending on level of tracing * Force SYCL RT to use specified backend when SYCL_BE is set. If SYCL_BE is not specified then SYCL RT is not forced to use specific backend. But make opencl backend preferred. * Update docs with info about SYCL_BE and SYCL_PI_TRACE Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

romanovvlad · 2020-04-22T19:08:10Z

sycl/source/detail/config.hpp

+  static backend *get() {
+    static bool Initialized = false;
+    static bool IsSet = false;
+    static backend Backend = backend::opencl;


Please, consider having backend* initialized to nullptr here.

romanovvlad · 2020-04-22T19:11:54Z

sycl/source/detail/config.hpp

+      return IsSet ? &Backend : nullptr;
+
+    const char *ValStr = BaseT::getRawValue();
+    const std::map<std::string, backend> SyclBeMap{


Please, consider using std::array.

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

romanovvlad

Please, do not force push as it kills possibility to track changes.

kbobrovs

sycl/source/detail/program_manager/program_manager.cpp
sycl/doc/EnvironmentVariables.md
LGTM

…_docs * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536)

…versioning * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536) [SYCL][CUDA] Move interop tests (intel#1570) [Driver][SYCL] Remove COFF object format designator for Windows device compiles (intel#1574) [SYCL] Fix conflicting visibility attributes (intel#1571) [SYCL][DOC] Update the SYCL Runtime Interface document with design details (intel#680) [SYCL] Improve image accessors support on a host device (intel#1502) [SYCL] Make queue's non-USM event ownership temporary (intel#1561) [SYCL] Added support of rounding modes for non-host devices (intel#1463) [SYCL] SemaSYCL significant refactoring (intel#1517) [SYCL] Support 0-dim accessor in handler::copy(accessor, accessor) (intel#1551)

`cl_*` types are defined by SYCL spec as interoperability interface with OpenCL. There is no need to use them in most of SYCL tests. This change is mostly motivated by the fact that those `cl_*` type aliases were moved from `sycl` into `sycl::opencl` namespace: we simply want to avoid tests breakage when we align the implementation with the spec.

againull requested review from bader and romanovvlad as code owners April 7, 2020 23:59

againull requested review from garimagu and smaslov-intel April 8, 2020 00:00

smaslov-intel reviewed Apr 8, 2020

View reviewed changes

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

smaslov-intel reviewed Apr 8, 2020

View reviewed changes

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

smaslov-intel reviewed Apr 8, 2020

View reviewed changes

sycl/source/device_selector.cpp Outdated Show resolved Hide resolved

smaslov-intel reviewed Apr 8, 2020

View reviewed changes

hiaselhans mentioned this pull request Apr 8, 2020

[SYCL][PI] get backend type from Plugin, not env #1477

Closed

romanovvlad requested changes Apr 8, 2020

View reviewed changes

Ruyk reviewed Apr 10, 2020

View reviewed changes

smaslov-intel suggested changes Apr 10, 2020

View reviewed changes

againull force-pushed the multiple_plugins branch 2 times, most recently from 69922a9 to bee65ad Compare April 13, 2020 08:04

hiaselhans reviewed Apr 13, 2020

View reviewed changes

againull force-pushed the multiple_plugins branch 2 times, most recently from 981cb93 to 3296134 Compare April 13, 2020 18:13

keryell reviewed Apr 13, 2020

View reviewed changes

smaslov-intel suggested changes Apr 14, 2020

View reviewed changes

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

sycl/source/device_selector.cpp Outdated Show resolved Hide resolved

againull force-pushed the multiple_plugins branch from 4aab980 to 2a94663 Compare April 14, 2020 04:10

againull mentioned this pull request Apr 14, 2020

[SYCL] Make SYCL_BE force selection of specified BE if there are multiple choices #1522

Closed

againull requested review from romanovvlad and smaslov-intel April 14, 2020 21:40

romanovvlad requested changes Apr 15, 2020

View reviewed changes

againull force-pushed the multiple_plugins branch from 859305d to 2d89167 Compare April 22, 2020 00:30

againull requested review from kbobrovs, pvchupin and a team as code owners April 22, 2020 00:30

smaslov-intel reviewed Apr 22, 2020

View reviewed changes

againull force-pushed the multiple_plugins branch from 7109cd8 to c8e1512 Compare April 22, 2020 07:31

smaslov-intel previously approved these changes Apr 22, 2020

View reviewed changes

againull dismissed smaslov-intel’s stale review via 682a0a0 April 22, 2020 17:51

againull force-pushed the multiple_plugins branch from c8e1512 to 682a0a0 Compare April 22, 2020 17:51

againull added 4 commits April 22, 2020 11:48

Follow-up fixes

67d9f2a

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

Fix subdevice_pi test

d27517d

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

smaslov-intel previously approved these changes Apr 22, 2020

View reviewed changes

romanovvlad requested changes Apr 22, 2020

View reviewed changes

Address comments

cb7b4eb

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

againull dismissed smaslov-intel’s stale review via cb7b4eb April 22, 2020 20:48

againull force-pushed the multiple_plugins branch from 682a0a0 to cb7b4eb Compare April 22, 2020 20:48

Include missed headers

3677f67

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

againull requested a review from romanovvlad April 22, 2020 23:44

bader requested a review from smaslov-intel April 23, 2020 09:41

romanovvlad approved these changes Apr 23, 2020

View reviewed changes

Ruyk mentioned this pull request Apr 23, 2020

[SYCL][CUDA] Minor fixes required to run BabelStream benchmarks on CUDA #1543

Closed

smaslov-intel approved these changes Apr 23, 2020

View reviewed changes

pvchupin approved these changes Apr 23, 2020

View reviewed changes

kbobrovs approved these changes Apr 23, 2020

View reviewed changes

bader merged commit 937fec1 into intel:sycl Apr 23, 2020

hiaselhans mentioned this pull request Apr 25, 2020

[CUDA] illegal memory access from cuda_piEnqueueMemBufferRead #1473

Closed

againull deleted the multiple_plugins branch December 3, 2022 00:03

	static constexpr char SYCL_PI_TRACE[] = "SYCL_PI_TRACE";
	static constexpr auto SYCL_PI_TRACE = "SYCL_PI_TRACE";

	int dev_score = operator()(dev);
	int dev_score = (*this)(dev);

[SYCL] Support connection with multiple plugins #1490

[SYCL] Support connection with multiple plugins #1490

Conversation

againull commented Apr 7, 2020 • edited Loading

smaslov-intel left a 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

againull Apr 15, 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

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

Choose a reason for hiding this comment

smaslov-intel left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaslov-intel left a comment

Choose a reason for hiding this comment

bader commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanovvlad left a comment

Choose a reason for hiding this comment

kbobrovs left a comment

Choose a reason for hiding this comment

againull commented Apr 7, 2020 •

edited

Loading

againull Apr 15, 2020 •

edited

Loading