[SYCL] Add ONEAPI_DEVICE_SELECTOR implementation #6779

cperkinsintel · 2022-09-14T03:12:30Z

Functionally complete. Needs tests and possible reorganization.

sycl/source/detail/platform_impl.cpp

sycl/include/sycl/detail/device_filter.hpp

gmlueck · 2022-09-14T20:35:55Z

I assume we should change EnvironmentVariables.md to document the new envvar?

…ct with a P. Signed-off-by: Chris Perkins <[email protected]>

cperkinsintel · 2022-09-14T21:57:49Z

@gmlueck - good catch. I've updated the EnvironmentVariables.md file.

sycl/doc/EnvironmentVariables.md

sycl/source/detail/device_filter.cpp

…ld have been separate commits.

…pelling correction. To my mind, ready for review.

cperkinsintel · 2022-09-16T22:31:29Z

/verify with intel/llvm-test-suite#1270

steffenlarsen

Only some minor comments.

sycl/doc/EnvironmentVariables.md

sycl/source/detail/device_filter.cpp

sycl/source/detail/platform_impl.cpp

…s fixed.

steffenlarsen

LGTM!

cperkinsintel · 2022-09-23T15:48:06Z

@gmlueck - I think this is all ready. Needs your ok for any furtherance.

sycl/doc/EnvironmentVariables.md

gmlueck · 2022-09-23T21:55:38Z

sycl/doc/EnvironmentVariables.md

+| `ONEAPI_DEVICE_SELECTOR=hip:0,2` | Devices with numeric identifier of 0 and 2 are chosen from the HIP platform. |
+| `ONEAPI_DEVICE_SELECTOR=opencl:0.*` | All the sub-devices (aka tiles) from the OpenCL device identified by 0 will be available. |
+| `ONEAPI_DEVICE_SELECTOR=opencl:0.2` | The third tile (2 in 0 based counting) of the OpenCL device with numeric identifier 0 will be the sole device available.  |
+| `ONEAPI_DEVICE_SELECTOR=level_zero:gpu,gpu.*,gpu.*.*` | A wide net is cast. All level_zero gpus will be availabe in the device list, along with all their child partitions (if any), as well as the next level of partitions below them.  It would be up to the developer to figure out which device is which, if that is important. (This can be done with `info::device::parent_device`)|


I think we decided against the gpu.* syntax in the end in favor of *.*.

The part about "This can be done with info::device::parent_device" seems wrong to me. All the devices selected by ONEAPI_DEVICE_SELECTOR will be SYCL root devices, so none of them will have a parent device.

gpu.* selects not all the GPUs, but all partitions of any partitionable GPU.

All the devices selected by ONEAPI_DEVICE_SELECTOR will be SYCL root devices, so none of them will have a parent device.

That is definitely not what is happening. The ONEAPI_DEVICE_SELECTOR limits the available choice of devices, and in the case of sub-devices, exposes partitions in the listing of devices gotten. They are not modified in any way. So while a partition of some parent device may now be gotten via device::get_devices() or platform::get_devices(), which is new, the get_info() applied against those devices would not be modified. If a device is a partition, I presume it'll have a parent, which the users code could get.

It's cool and all that ONEAPI_DEVICE_SELECTOR can help free users from the burden of having to go and root around partitioning before they can even run a kernel, but pretending that a partition is not actually a partition seems ill-advised.

I made a gist that demos this (needs this PR for the ONEAPI_DEVICE_SELECTOR support, and a machine with a partitionable GPU): https://gist.github.com/cperkinsintel/a14cdf1666c474f428cbdbc5ab98272b

gpu.* selects not all the GPUs, but all partitions of any partitionable GPU.

This is also what *.* does, which is why we dropped gpu.*.

That is definitely not what is happening. The ONEAPI_DEVICE_SELECTOR limits the available choice of devices, and in the case of sub-devices, exposes partitions in the listing of devices gotten. They are not modified in any way. So while a partition of some parent device may now be gotten via device::get_devices() or platform::get_devices(), which is new, the get_info() applied against those devices would not be modified. If a device is a partition, I presume it'll have a parent, which the users code could get.

This is contrary to the SYCL specification. The specification of device::get_devices() is:

Returns a std::vector containing all the root devices from all SYCL backends available in the system
which have the device type encapsulated by deviceType.

Note that it specifically says "root devices", which is defined in the glossary as:

A device that is not a sub-device. The function device::get_devices() returns a vector of all the root devices.

And the spec says that devices that are not a sub-device do not have any parent. Here is the specification for info::device::parent_device:

Returns the parent SYCL device to which this sub-device is a child if this is a sub-device. Must throw an exception with the errc::invalid error code if this SYCL device is not a sub device.

The intention of the "dot" syntax is to allow programmers pretend that each tile is an independent root device. Therefore, we should maintain this illusion and not expose any parent device.

@gmlueck *.* is any partitionable device ( caveat affinity doman, next_partitionable) ,whereas gpu.* is any partitionable GPU (same caveats), correct? The fact may be that today those two sets are equal, but in the future, they might not be. For example, it is conceivable that a CPU device could be support such partitioning.

In any case, both are consistent with the BNF, no?

The intention of the "dot" syntax is to allow programmers pretend that each tile is an independent root device. Therefore, we should maintain this illusion and not expose any parent device.

So you are saying that if the user specifies ONEAPI_DEVICE_SELECTOR=level_zero:0,0.0 which would select two exact devices (the root device and one partition of it). That we should NOT let them disambiguate those two devices from each other? Something they would be able to do if they didn't use ONEAPI_DEVICE_SELECTOR ? To my mind, we are treading out of the "enable the programmer" zone by helping them into the "limit the programmer" zone by pretending we know better than them. How on earth are we helping anyone by changing the value of get_info<info::device::parent>() from it's real value to a pretend one?

I see it as a tension between the spec and ONEAPI_DEVICE_SELECTOR which is, obviously, outside the specification. In the strictest sense, what ONEAPI_DEVICE_SELECTOR proposes to do via sub-devices is a violation of the spec. To my mind, that's is it's utility, it's why we are developing it. The boundaries the specification put up now make it difficult and tedious for users to work with partitions. ONEAPI_DEVICE_SELECTOR eases that, but its use exceeds the spec as it exists now, and that seems OK, to me at least.

sycl/doc/EnvironmentVariables.md

keryell

Interesting.
Let's wait for C++20 ranges to generalize to any level of sub-devices. :-)

sycl/include/sycl/detail/device_filter.hpp

keryell · 2022-09-26T11:57:59Z

sycl/source/detail/device_filter.cpp

+    if (SubSubDeviceStr[0] == '*') {
+      Target.HasSubSubDeviceWildCard = true;
+    } else {
+      std::string SSDS(SubSubDeviceStr);


Not clear why this intermediate copy.

can't do stoi on string_view. But I removed the stringstream for string concatenation.

sycl/source/detail/device_filter.cpp

sycl/doc/EnvironmentVariables.md

Signed-off-by: Chris Perkins <[email protected]>

gmlueck

Sorry, just one minor thing. The rest of the doc changes LGTM.

sycl/doc/EnvironmentVariables.md

gmlueck

Doc changes LGTM.

cperkinsintel · 2022-10-06T00:46:08Z

The ESIMD failures are not from this PR. They are occurring with the main sycl branch as well.

cperkinsintel added 5 commits September 9, 2022 21:40

initial commit

a03226d

hook environment var up to SYCL.Config. Working.

98ee8ac

too fancy. checkpointing.

a004eff

done, modulo tests, testing and testacularization

5bd21e0

update message in sycl-ls

94b076d

steffenlarsen reviewed Sep 14, 2022

View reviewed changes

reviewer feedback, bug-fixes

55aaad8

updated environment variable docs. prose is perfect as written. perfe…

5dc0a12

…ct with a P. Signed-off-by: Chris Perkins <[email protected]>

cperkinsintel mentioned this pull request Sep 15, 2022

[SYCL] tests for ONEAPI_DEVICE_SELECTOR. intel/llvm-test-suite#1270

Merged

minor updates per feedback, changes

125fb95

Wee-Free-Scot reviewed Sep 16, 2022

View reviewed changes

sycl/doc/EnvironmentVariables.md Outdated Show resolved Hide resolved

Wee-Free-Scot reviewed Sep 16, 2022

View reviewed changes

sycl/source/detail/device_filter.cpp Show resolved Hide resolved

Wee-Free-Scot reviewed Sep 16, 2022

View reviewed changes

sycl/source/detail/device_filter.cpp Show resolved Hide resolved

cperkinsintel added 4 commits September 16, 2022 12:52

remove iostream, add fpga, and introduce std::optional. Probably shou…

e161400

…ld have been separate commits.

errors for parsing only, not device matching

de06630

clean up some comments

c99d8ea

potential errors output as messages now. and review feedback, and a s…

82cea81

…pelling correction. To my mind, ready for review.

cperkinsintel marked this pull request as ready for review September 16, 2022 22:30

cperkinsintel requested review from a team as code owners September 16, 2022 22:30

cperkinsintel requested a review from againull September 16, 2022 22:30

cperkinsintel added 2 commits September 19, 2022 15:09

minor fix for unit tests

7ed9934

warning when no matching devices

e9047dc

cperkinsintel changed the title ~~[WIP] ONEAPI_DEVICE_SELECTOR implementation~~ [SYCL]ONEAPI_DEVICE_SELECTOR implementation Sep 20, 2022

incorrect warning

62aa821

steffenlarsen reviewed Sep 20, 2022

View reviewed changes

cperkinsintel marked this pull request as draft September 20, 2022 23:10

cperkinsintel changed the title ~~[SYCL]ONEAPI_DEVICE_SELECTOR implementation~~ [WIP]ONEAPI_DEVICE_SELECTOR implementation Sep 20, 2022

cperkinsintel added 4 commits September 21, 2022 14:12

sub-sub device support added. device counting discrepancy with sycl-l…

68ffa39

…s fixed.

tidy

4c0ec48

resolved merge conflicts

f685ce2

reviewer feedback

b3b8a77

cperkinsintel marked this pull request as ready for review September 21, 2022 21:57

cperkinsintel changed the title ~~[WIP]ONEAPI_DEVICE_SELECTOR implementation~~ [SYCL] ONEAPI_DEVICE_SELECTOR implementation Sep 21, 2022

steffenlarsen approved these changes Sep 22, 2022

View reviewed changes

fixed windows build issue, updated docs with sub-sub info and example

9b8e468

gmlueck reviewed Sep 23, 2022

View reviewed changes

reviewer feedback to .md file

533677d

keryell suggested changes Sep 26, 2022

View reviewed changes

keryell reviewed Sep 26, 2022

View reviewed changes

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

bader changed the title ~~[SYCL] ONEAPI_DEVICE_SELECTOR implementation~~ [SYCL] Add ONEAPI_DEVICE_SELECTOR implementation Sep 27, 2022

cperkinsintel added 2 commits September 28, 2022 11:32

reviewer feedback

1e91fda

more restrictive parsing

41d6d3d

bader requested a review from keryell October 2, 2022 09:12

requested documentation changes

c59a7d9

gmlueck reviewed Oct 5, 2022

View reviewed changes

last doc feedback incorporated

da634ea

Signed-off-by: Chris Perkins <[email protected]>

cperkinsintel requested review from gmlueck and removed request for keryell October 5, 2022 20:56

gmlueck reviewed Oct 5, 2022

View reviewed changes

sycl/doc/EnvironmentVariables.md Outdated Show resolved Hide resolved

spaces added to Level Zero

db85931

gmlueck approved these changes Oct 5, 2022

View reviewed changes

Merge branch 'sycl' into cperkins-oneapi_device_selector

cc35153

steffenlarsen merged commit 28d0cd3 into intel:sycl Oct 6, 2022

[SYCL] Add ONEAPI_DEVICE_SELECTOR implementation #6779

[SYCL] Add ONEAPI_DEVICE_SELECTOR implementation #6779

Uh oh!

Conversation

cperkinsintel commented Sep 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmlueck commented Sep 14, 2022

Uh oh!

cperkinsintel commented Sep 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cperkinsintel commented Sep 16, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

cperkinsintel commented Sep 23, 2022

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!

cperkinsintel Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

cperkinsintel commented Oct 6, 2022

cperkinsintel Sep 27, 2022 •

edited

Loading