[SYCL] Fix devices available when SYCL_BE is set. #2125

bso-intel · 2020-07-16T04:51:54Z

Currently when SYCL_BE is set, other devices (such CPU) is filtered out from the available devices list.
This does not conform to the spec of device::get_devices().

static vector_classget_devices(info::device_type deviceType =info::device_type::all)	Returns a vector_class containing all SYCL devices available in the system of the device type specified by the parameter deviceType. The returned vector_class must contain at least a SYCL device that is a host device if the deviceType is info::device_type::all, or a single host device if the deviceType is info::device_type::host.

Basically, setting SYCL_BE should not limit the available devices (e.g., CPU) in the system.
The current implementation does not allow the usage of multiple devices when SYCL_BE is set because it filter out devices other than the only SYCL_BE setting device.

Signed-off-by: Byoungro So [email protected]

Signed-off-by: Byoungro So <[email protected]>

bader

Please, add a regression test.

bader · 2020-07-16T08:42:48Z

sycl/source/device_selector.cpp

@@ -21,11 +22,17 @@ namespace sycl {
 // Utility function to check if device is of the preferred backend.
 // Currently preference is given to the level0 backend.
 static bool isDeviceOfPreferredSyclBe(const device &Device) {


Suggested change

static bool isDeviceOfPreferredSyclBe(const device &Device) {

static bool isLevelZeroGPUDevice(const device &Device) {

New code below using this function with current generic name is very confusing. It seems like it should work with any device type and old code applied this function to all device type looks reasonable. Now it's applied only to GPU device type and it's not obvious why.

@bader This function is used to just give a bonus point (+50) when deciding which device to choose when multiple devices are available with the same score.
So, it is mainly used for GPU devices for now.
This function will give a bonus point if the device "Device" is matched with SYCL_BE. Not only for Level0. So your naming suggestion is not quite right.

isPreferredGPUDevice?

That makes more sense. :)

sycl/source/device_selector.cpp

romanovvlad

I would like that @againull and @smaslov-intel approve

romanovvlad · 2020-07-16T11:23:10Z

sycl/source/device_selector.cpp

    Score += 500;
+    // Give preference to device of SYCL BE.
+    if (isDeviceOfPreferredSyclBe(dev))


I think preferred BE should work for all devices, not only for GPU.

The current usage of this function is only used to give a bonus point for the device that is matched with SYCL_BE.
We don't need to give a bonus point for non-GPU device at this moment.

@romanovvlad
Current options for SYCL_BE are useful only for GPU devices.
When we have multiple BE options for other devices, we can remove this condition later.

smaslov-intel · 2020-07-16T12:26:23Z

I think we do need a way (SYCL_BE seems a good fit to me) to force SYCL RT do device discovery/selection among devices of specified backend only. The fact that this ninja setting overrides the default/standard behavior does not seem to be a problem.
Note, we are doing the same override with SYCL_DEVICE_TYPE:

| 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 people want to see everything (including seeing the same physical device multiple times through different backends) then they should just not use the SYCL_BE. The preferred BE is hardcoded for such cases, and I am not sure we need to give a way to control the preferred BE to end users, other than the more strict SYCL_BE.

smaslov-intel

I think we should instead make SYCL_BE even more strict and affect not only device selection, but device discovery as well.

Co-authored-by: Alexey Bader <[email protected]>

bso-intel · 2020-07-16T17:47:33Z

I think we do need a way (SYCL_BE seems a good fit to me) to force SYCL RT do device discovery/selection among devices of specified backend only. The fact that this ninja setting overrides the default/standard behavior does not seem to be a problem.
Note, we are doing the same override with SYCL_DEVICE_TYPE:

| 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 people want to see everything (including seeing the same physical device multiple times through different backends) then they should just not use the SYCL_BE. The preferred BE is hardcoded for such cases, and I am not sure we need to give a way to control the preferred BE to end users, other than the more strict SYCL_BE.

SYCL_BE is already honored by SYCL RT. SYCL RT uses its desired BE when choosing the plugin.
However, the current implementation in device.cpp is filitering out other devices already found in the system just because that device is not SYCL_BE type.
This is against the spec.
SYCL_BE is used to force (by giving more bonus point) the desired backend.
It is not intended to hide all other available devices, which is the current behavior.
Currently, this bug causes issues for users who want to use multiple device types for different kernels. For example, one kernel for CPU and one kernel for GPU. This is not possible in current implementation.

bso-intel · 2020-07-16T18:03:27Z

I think we should instead make SYCL_BE even more strict and affect not only device selection, but device discovery as well.

I am not sure what "more strict" means, but yes SYCL_BE is currently used to force SYCL RT to use the desired BE. This PR is not intended to relieve this behavior.
This PR is intended to fix the current behavior of filtering out all devices which are not SYCL_BE.

Signed-off-by: Byoungro So <[email protected]>

bso-intel · 2020-07-16T20:50:32Z

Please, add a regression test.

Done.

smaslov-intel · 2020-07-20T13:10:49Z

I am not sure what "more strict" means, but yes SYCL_BE is currently used to force SYCL RT to use the desired BE.

By more strict I mean make SYCL_BE force the devices of the specified BE only be discovered/selected. This way we'd provide deterministic behavior (unlike current prefer semantics) needed for robust testing scenarios, at least.

bso-intel · 2020-07-20T17:34:08Z

I am not sure what "more strict" means, but yes SYCL_BE is currently used to force SYCL RT to use the desired BE.

By more strict I mean make SYCL_BE force the devices of the specified BE only be discovered/selected. This way we'd provide deterministic behavior (unlike current prefer semantics) needed for robust testing scenarios, at least.

It is already deterministic. SYCL_BE is forced as long as the desired device is found on the system.
But it should not hide other available devices such as CPU.
Note that we should not disallow users from utilizing multiple computing resources in the system.
As I quoted above, the spec never mentioned that only one device is discovered.
@romanovvlad, @bader What do you think?

againull · 2020-07-20T22:04:08Z

I agree with comment form @smaslov-intel: #2125 (comment)

This PR changes logic of SYCL_BE from "force" to "prefer". But we already have "prefer" logic hardcoded in isDeviceOfPreferredSyclBe => we prefer level zero.
In other words this PR turns SYCL_BE to environment variable which just controls default backend and don't force anything.
Note. Probably you want to create new variable called SYCL_DEFAULT_BE or SYCL_PREFERRED_BE for this.

As a result, when someone runs application with SYCL_BE=PI_LEVEL0, it will not be possible to be sure (without tracing) which backend was really used. It can be even cuda backend if level0 and opencl runtimes are not available.

Currently, this bug causes issues for users who want to use multiple device types for different kernels. For example, one kernel for CPU and one kernel for GPU. This is not possible in current implementation.

I believe it is possible when SYCL_BE is not provided => preferred and available backend will be used for each device. Or when SYCL_BE=PI_OPENCL is provided, because opencl is supported for all devices - cpu, gpu, acc...
It is not possible when user provides SYCL_BE=PI_LEVEL0 or SYCL_BE=PI_CUDA, because these backends are not supported for CPU and this looks reasonable to me.

bso-intel · 2020-07-20T22:21:25Z

I agree with comment form @smaslov-intel: #2125 (comment)

This PR changes logic of SYCL_BE from "force" to "prefer". But we already have "prefer" logic hardcoded in isDeviceOfPreferredSyclBe => we prefer level zero.
In other words this PR turns SYCL_BE to environment variable which just controls default backend and don't force anything.
Note. Probably you want to create new variable called SYCL_DEFAULT_BE or SYCL_PREFERRED_BE for this.

As a result, when someone runs application with SYCL_BE=PI_LEVEL0, it will not be possible to be sure (without tracing) which backend was really used. It can be even cuda backend if level0 and opencl runtimes are not available.

Currently, this bug causes issues for users who want to use multiple device types for different kernels. For example, one kernel for CPU and one kernel for GPU. This is not possible in current implementation.

I believe it is possible when SYCL_BE is not provided => preferred and available backend will be used for each device. Or when SYCL_BE=PI_OPENCL is provided, because opencl is supported for all devices - cpu, gpu, acc...
It is not possible when user provides SYCL_BE=PI_LEVEL0 or SYCL_BE=PI_CUDA, because these backends are not supported for CPU and this looks reasonable to me.

When SYCL_BE is not set, all devices can be used.
When SYCL_BE is set, only the desired device can be used. This basically prevents users from utilizing GPU Level0 and CPU together.
Setting SYCL_BE should be irrelevant to the available devices according to the spec.

againull · 2020-07-20T22:32:52Z

When SYCL_BE is set, only the desired device can be used. This basically prevents users from utilizing GPU Level0 and CPU together.

I am quite sure, this statement is wrong for SYCL_BE=PI_OPENCL. CPU doesn't support level0, so when someone forces level0 backend I find it correct that CPU is not available.

Setting SYCL_BE should be irrelevant to the available devices according to the spec.

Sorry, but I am sure there is no such thing as SYCL_BE in the spec at all.

jbrodman · 2020-07-22T13:58:02Z

I strongly disagree with this PR. SYCL_BE is a hammer that is supposed to filter out all existence of devices that:

are not the host device
are not part of the specified BE.

If you don't want that behavior - don't use SYCL_BE.

When translating the `spirv.Extension` metadata of an LLVM Module, report an error when encountering an extension that has been explicitly disabled. Original commit: KhronosGroup/SPIRV-LLVM-Translator@2e917c9

[SYCL] Fix devices available when SYCL_BE is set.

8c3ae1a

Signed-off-by: Byoungro So <[email protected]>

bso-intel requested review from kbobrovs, pvchupin and a team as code owners July 16, 2020 04:51

bader requested a review from romanovvlad July 16, 2020 08:41

bader reviewed Jul 16, 2020

View reviewed changes

romanovvlad reviewed Jul 16, 2020

View reviewed changes

romanovvlad requested review from againull and smaslov-intel July 16, 2020 11:23

smaslov-intel suggested changes Jul 16, 2020

View reviewed changes

Update sycl/source/device_selector.cpp

7bbc4cd

Co-authored-by: Alexey Bader <[email protected]>

bso-intel added 2 commits July 16, 2020 13:44

Added a regression test and changed the function name

8dcae2b

Signed-off-by: Byoungro So <[email protected]>

fixed clang-format

a1c005b

Signed-off-by: Byoungro So <[email protected]>

jbrodman self-requested a review July 22, 2020 14:00

bso-intel closed this Aug 8, 2020

bso-intel deleted the device-sycl-be branch August 8, 2020 22:32

	static bool isDeviceOfPreferredSyclBe(const device &Device) {
	static bool isLevelZeroGPUDevice(const device &Device) {

[SYCL] Fix devices available when SYCL_BE is set. #2125

[SYCL] Fix devices available when SYCL_BE is set. #2125

Uh oh!

Conversation

bso-intel commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

bader Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

bso-intel Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

bader Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

bso-intel Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romanovvlad left a comment

Choose a reason for hiding this comment

Uh oh!

romanovvlad Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

bso-intel Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

bso-intel Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

smaslov-intel commented Jul 16, 2020

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

bso-intel commented Jul 16, 2020

Uh oh!

bso-intel commented Jul 16, 2020

Uh oh!

bso-intel commented Jul 16, 2020

Uh oh!

smaslov-intel commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bso-intel commented Jul 20, 2020

Uh oh!

againull commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bso-intel commented Jul 20, 2020

Uh oh!

againull commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbrodman commented Jul 22, 2020

Uh oh!

Uh oh!

bso-intel commented Jul 16, 2020 •

edited

Loading

smaslov-intel commented Jul 20, 2020 •

edited

Loading

againull commented Jul 20, 2020 •

edited

Loading

againull commented Jul 20, 2020 •

edited

Loading