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;

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

Fixed.

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.

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.

Done

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.

Fixed.

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

Yes, my mistake, fixed.

seems simpler

Perhaps an .emplace_back could work here?

looks clearer to me

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...

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.

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.

Please, consider having backend* initialized to nullptr here.

Please, consider using std::array.