Address review comments

* 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 <[email protected]>

Author: againull

sycl/doc/EnvironmentVariables.md

@@ -11,8 +11,8 @@ subject to change. Do not rely on these variables in production code.
 
 | Environment variable | Values | Description |
 | -------------------- | ------ | ----------- |
-| SYCL_PI_TRACE | Any(\*) | Force tracing of PI calls to stderr. |
-| SYCL_BE | PI_OPENCL, PI_CUDA, PI_OTHER | When SYCL RT is built with PI, this controls which plugin is used by the default device selector. Default value is PI_OPENCL. |
+| SYCL_PI_TRACE | Described [below](#sycl_pi_trace-options)  | Enable specified level of tracing for PI. |
+| SYCL_BE | PI_OPENCL, PI_CUDA | When SYCL RT is built with PI, force SYCL to consider only devices of the specified backend during the device selection. |
 | SYCL_DEVICE_TYPE | One of: CPU, GPU, ACC, HOST | Force SYCL to use the specified device type. If unset, default selection rules are applied. If set to any unlisted value, this control has no effect. If the requested device type is not found, a `cl::sycl::runtime_error` exception is thrown. If a non-default device selector is used, a device must satisfy both the selector and this control to be chosen. This control only has effect on devices created with a selector. |
 | SYCL_PROGRAM_COMPILE_OPTIONS | String of valid OpenCL compile options | Override compile options for all programs. |
 | SYCL_PROGRAM_LINK_OPTIONS | String of valid OpenCL link options | Override link options for all programs. |
@@ -39,3 +39,12 @@ SYCL_PRINT_EXECUTION_GRAPH can accept one or more comma separated values from th
 | after_addHostAcc | print graph after addHostAccessor method |
 | always | print graph before and after each of the above methods |
 
+### SYCL_PI_TRACE Options
+
+SYCL_PI_TRACE can accept one of the values from the table below
+
+| Option | Description |
+| ------ | ----------- |
+| 1 | Enable basic tracing |
+| 2 | Enable tracing of the PI calls |
+| -1 | Enable all levelis of tracing |

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

@@ -13,6 +13,7 @@
 
 #pragma once
 
+#include <CL/sycl/backend_types.hpp>
 #include <CL/sycl/detail/common.hpp>
 #include <CL/sycl/detail/export.hpp>
 #include <CL/sycl/detail/os_util.hpp>
@@ -124,13 +125,6 @@ void *loadOsLibrary(const std::string &Library);
 // library, implementation is OS dependent.
 void *getOsLibraryFuncAddress(void *Library, const std::string &FunctionName);
 
-// For selection of SYCL RT back-end, now manually through the "SYCL_BE"
-// environment variable.
-enum Backend { SYCL_BE_PI_OPENCL, SYCL_BE_PI_CUDA, SYCL_BE_PI_OTHER };
-
-// Get the preferred BE (selected with SYCL_BE).
-Backend getPreferredBE();
-
 // Get a string representing a _pi_platform_info enum
 std::string platformInfoToString(pi_platform_info info);
 

sycl/source/detail/config.cpp

@@ -103,7 +103,7 @@ void readConfig() {
 void dumpConfig() {
 #define CONFIG(Name, MaxSize, CompileTimeDef)                                  \
   {                                                                            \
-    const char *Val = SYCLConfig<Name>::get();                                 \
+    const char *Val = SYCLConfigBase<Name>::getRawValue();                     \
     std::cerr << SYCLConfigBase<Name>::MConfigName << " : "                    \
               << (Val ? Val : "unset") << std::endl;                           \
   }

sycl/source/detail/config.def

@@ -13,3 +13,5 @@
 CONFIG(SYCL_PRINT_EXECUTION_GRAPH, 32, __SYCL_PRINT_EXECUTION_GRAPH)
 CONFIG(SYCL_DISABLE_EXECUTION_GRAPH_CLEANUP, 1, __SYCL_DISABLE_EXECUTION_GRAPH_CLEANUP)
 CONFIG(SYCL_DEVICE_ALLOWLIST, 1024, __SYCL_DEVICE_ALLOWLIST)
+CONFIG(SYCL_BE, 16, __SYCL_BE)
+CONFIG(SYCL_PI_TRACE, 4, __SYCL_PI_TRACE)

sycl/source/detail/config.hpp

@@ -8,9 +8,12 @@
 
 #pragma once
 
+#include <CL/sycl/backend_types.hpp>
 #include <CL/sycl/detail/defines.hpp>
+#include <CL/sycl/detail/pi.hpp>
 
 #include <cstdlib>
+#include <map>
 
 __SYCL_INLINE_NAMESPACE(cl) {
 namespace sycl {
@@ -48,6 +51,9 @@ constexpr const char *getStrOrNullptr(const char *Str) {
   return (Str[0] == '_' && Str[1] == '_') ? nullptr : Str;
 }
 
+// Intializes configs from the configuration file
+void readConfig();
+
 template <ConfigID Config> class SYCLConfigBase;
 
 #define CONFIG(Name, MaxSize, CompileTimeDef)                                  \
@@ -65,38 +71,82 @@ template <ConfigID Config> class SYCLConfigBase;
      * beginning of the string, if it starts with double underscore(__) the    \
      * value is not set.*/                                                     \
     static const char *const MCompileTimeDef;                                  \
+                                                                               \
+    static const char *getRawValue() {                                         \
+      if (ConfigFromEnvEnabled)                                                \
+        if (const char *ValStr = getenv(MConfigName))                          \
+          return ValStr;                                                       \
+                                                                               \
+      if (ConfigFromFileEnabled) {                                             \
+        readConfig();                                                          \
+        if (MValueFromFile)                                                    \
+          return MValueFromFile;                                               \
+      }                                                                        \
+                                                                               \
+      if (ConfigFromCompileDefEnabled && MCompileTimeDef)                      \
+        return MCompileTimeDef;                                                \
+                                                                               \
+      return nullptr;                                                          \
+    }                                                                          \
   };
 #include "config.def"
 #undef CONFIG
 
-// Intializes configs from the configuration file
-void readConfig();
-
 template <ConfigID Config> class SYCLConfig {
   using BaseT = SYCLConfigBase<Config>;
 
 public:
   static const char *get() {
-    const char *ValStr = getRawValue();
+    const char *ValStr = BaseT::getRawValue();
     return ValStr;
   }
+};
 
-private:
-  static const char *getRawValue() {
-    if (ConfigFromEnvEnabled)
-      if (const char *ValStr = getenv(BaseT::MConfigName))
-        return ValStr;
+template <> class SYCLConfig<SYCL_BE> {
+  using BaseT = SYCLConfigBase<SYCL_BE>;
 
-    if (ConfigFromFileEnabled) {
-      readConfig();
-      if (BaseT::MValueFromFile)
-        return BaseT::MValueFromFile;
+public:
+  static backend get() {
+    static bool Initialized = false;
+    static backend Backend = backend::opencl;
+
+    // Configuration parameters are processed only once, like reading a string
+    // from environment and converting it into a typed object.
+    if (Initialized)
+      return Backend;
+
+    const char *ValStr = BaseT::getRawValue();
+    const std::map<std::string, backend> SyclBeMap{
+        {"PI_OPENCL", backend::opencl}, {"PI_CUDA", backend::cuda}};
+    if (ValStr) {
+      auto It = SyclBeMap.find(ValStr);
+      if (It == SyclBeMap.end())
+        pi::die("Invalid backend. "
+                "Valid values are PI_OPENCL/PI_CUDA");
+      Backend = It->second;
     }
+    Initialized = true;
+    return Backend;
+  }
+};
 
-    if (ConfigFromCompileDefEnabled && BaseT::MCompileTimeDef)
-      return BaseT::MCompileTimeDef;
+template <> class SYCLConfig<SYCL_PI_TRACE> {
+  using BaseT = SYCLConfigBase<SYCL_PI_TRACE>;
 
-    return nullptr;
+public:
+  static int get() {
+    static bool Initialized = false;
+    static int Level = 0; // No tracing by default
+
+    // Configuration parameters are processed only once, like reading a string
+    // from environment and converting it into a typed object.
+    if (Initialized)
+      return Level;
+
+    const char *ValStr = BaseT::getRawValue();
+    Level = (ValStr ? std::atoi(ValStr) : 0);
+    Initialized = true;
+    return Level;
   }
 };
 

sycl/source/detail/pi.cpp

@@ -15,6 +15,7 @@
 #include <CL/sycl/context.hpp>
 #include <CL/sycl/detail/common.hpp>
 #include <CL/sycl/detail/pi.hpp>
+#include <detail/config.hpp>
 #include <detail/plugin.hpp>
 
 #include <bitset>
@@ -141,80 +142,21 @@ std::string memFlagsToString(pi_mem_flags Flags) {
   return Sstream.str();
 }
 
-// A singleton class to aid that PI configuration parameters
-// 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 {
-  static Config *m_Instance;
-  T m_Data;
-  Config();
-
-public:
-  static T get() {
-    if (!m_Instance) {
-      m_Instance = new Config();
-    }
-    return m_Instance->m_Data;
-  }
-};
-
-template <typename T, const char *E>
-Config<T, E> *Config<T, E>::m_Instance = nullptr;
-
-// Lists valid configuration 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";
-
-// SYCL_PI_TRACE gives the mask of enabled tracing components (0 default)
-template <> Config<int, SYCL_PI_TRACE>::Config() {
-  const char *Env = std::getenv(SYCL_PI_TRACE);
-  m_Data = (Env ? std::atoi(Env) : 0);
-}
-
-static Backend getBE(const char *EnvVar) {
-  const char *BE = std::getenv(EnvVar);
-  const std::map<std::string, Backend> SyclBeMap{
-      {"PI_OTHER", SYCL_BE_PI_OTHER},
-      {"PI_CUDA", SYCL_BE_PI_CUDA},
-      {"PI_OPENCL", SYCL_BE_PI_OPENCL}};
-  if (BE) {
-    auto It = SyclBeMap.find(BE);
-    if (It == SyclBeMap.end())
-      pi::die("Invalid backend. "
-              "Valid values are PI_OPENCL/PI_CUDA");
-    return It->second;
-  }
-  // Default backend
-  return SYCL_BE_PI_OPENCL;
-}
-
-template <> Config<Backend, SYCL_BE>::Config() { m_Data = getBE(SYCL_BE); }
-
-// SYCL_INTEROP_BE is a way to specify the interoperability plugin.
-template <> Config<Backend, SYCL_INTEROP_BE>::Config() {
-  m_Data = getBE(SYCL_INTEROP_BE);
-}
-
-// Helper interface to not expose "pi::Config" outside of pi.cpp
-Backend getPreferredBE() { return Config<Backend, SYCL_BE>::get(); }
-
 // GlobalPlugin is a global Plugin used with Interoperability constructors that
 // use OpenCL objects to construct SYCL class objects.
 std::shared_ptr<plugin> GlobalPlugin;
 
 // Find the plugin at the appropriate location and return the location.
-bool findPlugins(vector_class<std::pair<std::string, Backend>> &PluginNames) {
+bool findPlugins(vector_class<std::pair<std::string, backend>> &PluginNames) {
   // TODO: Based on final design discussions, change the location where the
   // plugin must be searched; how to identify the plugins etc. Currently the
   // search is done for libpi_opencl.so/pi_opencl.dll file in LD_LIBRARY_PATH
   // env only.
   //
-  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>(OPENCL_PLUGIN_NAME,
+                                                             backend::opencl));
   PluginNames.push_back(
-      std::make_pair<std::string, Backend>(CUDA_PLUGIN_NAME, SYCL_BE_PI_CUDA));
+      std::make_pair<std::string, backend>(CUDA_PLUGIN_NAME, backend::cuda));
   return true;
 }
 
@@ -249,12 +191,12 @@ bool bindPlugin(void *Library, PiPlugin *PluginInformation) {
 }
 
 bool trace(TraceLevel Level) {
-  auto TraceLevelMask = Config<int, SYCL_PI_TRACE>::get();
+  auto TraceLevelMask = SYCLConfig<SYCL_PI_TRACE>::get();
   return (TraceLevelMask & Level) == Level;
 }
 
 const char *traceLabel() {
-  auto TraceLevelMask = Config<int, SYCL_PI_TRACE>::get();
+  int TraceLevelMask = SYCLConfig<SYCL_PI_TRACE>::get();
   switch (TraceLevelMask) {
   case PI_TRACE_BASIC:
     return "SYCL_PI_TRACE[PI_TRACE_BASIC]: ";
@@ -271,7 +213,7 @@ const char *traceLabel() {
 // Initializes all available Plugins.
 vector_class<plugin> initialize() {
   vector_class<plugin> Plugins;
-  vector_class<std::pair<std::string, Backend>> PluginNames;
+  vector_class<std::pair<std::string, backend>> PluginNames;
   findPlugins(PluginNames);
 
   if (PluginNames.empty() && trace(PI_TRACE_ALL))
@@ -297,12 +239,16 @@ vector_class<plugin> initialize() {
       }
       continue;
     }
-    // Set the Global Plugin based on SYCL_INTEROP_BE.
-    // Rework this when it will be explicit in the code which BE is used in the
-    // interoperability methods.
-    if (Config<Backend, SYCL_INTEROP_BE>::get() == PluginNames[I].second) {
+    if (SYCLConfig<SYCL_BE>::get() == backend::opencl &&
+        PluginNames[I].first.find("opencl") != std::string::npos) {
+      // Use the OpenCL plugin as the GlobalPlugin
       GlobalPlugin =
-          std::make_shared<plugin>(PluginInformation, PluginNames[I].second);
+          std::make_shared<plugin>(PluginInformation, backend::opencl);
+    }
+    if (SYCLConfig<SYCL_BE>::get() == backend::cuda &&
+        PluginNames[I].first.find("cuda") != std::string::npos) {
+      // Use the CUDA plugin as the GlobalPlugin
+      GlobalPlugin = std::make_shared<plugin>(PluginInformation, backend::cuda);
     }
     Plugins.emplace_back(plugin(PluginInformation, PluginNames[I].second));
     if (trace(TraceLevel::PI_TRACE_BASIC))

sycl/source/detail/plugin.hpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #pragma once
+#include <CL/sycl/backend_types.hpp>
 #include <CL/sycl/detail/common.hpp>
 #include <CL/sycl/detail/pi.hpp>
 #include <CL/sycl/stl.hpp>
@@ -23,7 +24,7 @@ class plugin {
 public:
   plugin() = delete;
 
-  plugin(RT::PiPlugin Plugin, RT::Backend UseBackend)
+  plugin(RT::PiPlugin Plugin, backend UseBackend)
       : MPlugin(Plugin), MBackend(UseBackend) {}
 
   ~plugin() = default;
@@ -73,11 +74,11 @@ class plugin {
     checkPiResult(Err);
   }
 
-  RT::Backend getBackend(void) const { return MBackend; }
+  backend getBackend(void) const { return MBackend; }
 
 private:
   RT::PiPlugin MPlugin;
-  const RT::Backend MBackend;
+  const backend MBackend;
 }; // class plugin
 } // namespace detail
 } // namespace sycl

sycl/source/detail/program_manager/program_manager.cpp

@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include <CL/sycl/backend_types.hpp>
 #include <CL/sycl/context.hpp>
 #include <CL/sycl/detail/common.hpp>
 #include <CL/sycl/detail/os_util.hpp>
@@ -270,8 +271,8 @@ static bool isDeviceBinaryTypeSupported(const context &C,
   }
 
   // OpenCL 2.1 and greater require clCreateProgramWithIL
-  pi::Backend CBackend = (detail::getSyclObjImpl(C)->getPlugin()).getBackend();
-  if ((CBackend == pi::SYCL_BE_PI_OPENCL) &&
+  backend CBackend = (detail::getSyclObjImpl(C)->getPlugin()).getBackend();
+  if ((CBackend == backend::opencl) &&
       C.get_platform().get_info<info::platform::version>() >= "2.1")
     return true;
 

sycl/source/detail/scheduler/commands.cpp

@@ -10,6 +10,7 @@
 
 #include "CL/sycl/access/access.hpp"
 #include <CL/cl.h>
+#include <CL/sycl/backend_types.hpp>
 #include <CL/sycl/detail/kernel_desc.hpp>
 #include <CL/sycl/detail/memory_manager.hpp>
 #include <CL/sycl/detail/stream_impl.hpp>
@@ -1672,7 +1673,7 @@ cl_int ExecCGCommand::enqueueImp() {
         Requirement *Req = (Requirement *)(Arg.MPtr);
         AllocaCommandBase *AllocaCmd = getAllocaForReq(Req);
         RT::PiMem MemArg = (RT::PiMem)AllocaCmd->getMemAllocation();
-        if (Plugin.getBackend() == (pi::Backend::SYCL_BE_PI_OPENCL)) {
+        if (Plugin.getBackend() == backend::opencl) {
           Plugin.call<PiApiKind::piKernelSetArg>(Kernel, Arg.MIndex,
                                                  sizeof(RT::PiMem), &MemArg);
         } else {