[SYCL] SYCL 2020 callable device selectors #6486

cperkinsintel · 2022-07-28T01:07:32Z

In SYCL 2020 device selectors switch from subclasses to simply being any callable. This allows user defined device selector lambdas and more. This implements them for the queue, platform and device classes.

Tests are updated here: intel/llvm-test-suite#1119

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

…redDevices in scoring and pi-trace disbled. Signed-off-by: Chris Perkins <[email protected]>

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

… return type (as opposed to devicePtr). Signed-off-by: Chris Perkins <[email protected]>

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

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

cperkinsintel · 2022-07-28T04:05:45Z

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

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

cperkinsintel · 2022-07-28T21:53:06Z

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

sycl/test/abi/sycl_symbols_windows.dump

sycl/source/device_selector.cpp

sycl/include/sycl/device_selector.hpp

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

keryell

Nice to see at last some real SYCL 2020 device selectors. \o/

sycl/include/sycl/device.hpp

sycl/include/sycl/platform.hpp

sycl/include/sycl/queue.hpp

keryell · 2022-08-02T23:12:06Z

sycl/source/device_selector.cpp

+
+} // namespace detail
+
+device device_selector::select_device() const {


Do you need to implement the old SYCL 1.2.1 ugly Java-factory style API ?

I'm no fan either, but this is still used if compiling with C++14. Just preserving the functionality that was here before.

sycl/source/device_selector.cpp

keryell · 2022-08-02T23:30:58Z

sycl/include/sycl/queue.hpp

+  explicit queue(const DeviceSelector &deviceSelector,
+                 const async_handler &AsyncHandler,
+                 const property_list &PropList = {})
+      : queue(detail::select_device(deviceSelector), AsyncHandler, PropList) {}


Suggested change

: queue(detail::select_device(deviceSelector), AsyncHandler, PropList) {}

: queue(device { deviceSelector }, AsyncHandler, PropList) {}

?

@keryell I like this suggestion, and if you prefer it I'll change it.

However, consider that of the four constructors we are adding here, only two are amenable to the change. The other two queue constructors use continue to use detail::select_device because they have the additional context argument. Personally, I prefer all four to be consistent with each other, each calling detail::select_device directly, rather than just passing it to the device constructor in two cases. This may simply be a matter of taste. Let me know.

Interesting you need this detail::select_device at the first place.
Reading again the specification on queue, queue creation throws if the device picked by the selector is not in the context. The device is not the best one picked from the devices in the context but it is picked independently. So it is unclear why this detail::select_device takes all these parametersn

My reading was that the device selector would be used to select the best device from the context (assuming an acceptable one could be found). It makes little sense to even provide that constructor otherwise.

FWIW, that is how the analog SYCL 1.2.1 queue constructor is implemented right now, and I used its behavior to model this one ( https://github.com/intel/llvm/blob/sycl/sycl/source/queue.cpp#L23-L30 )

Interesting feature, but this is not how the SYCL specification is defined:

Constructs a SYCL queue instance that is associated with the syclContext provided, using the device returned by the device selector provided. Must throw an exception with the errc::invalid error code if syclContext does not encapsulate the SYCL device returned by deviceSelector.

So either the specification has to be changed or DPC++ has to be updated. @bader any direction?
But anyway, this is outside of this PR.

I agree with @keryell that device selection is supposed to be "context-free". I don't know if logic currently implemented by DPC++ is used, but it can be implemented outside of the library.
To honest, I don't have enough data to recommend any direction here.
At first glace, DPC++ behavior sounds like a user friendly approach. The runtime emits less exceptions for the user to handle and tries to do it best to select the right device. On the other hand, it's just a syntax sugar, which might add an overhead and not sure if it's really used in the real word.
@intel/dpcpp-specification-reviewers, do you have any thoughts on that subject?

I looked at all the places in the SYCL 2020 spec where a device selector is passed. In all other cases, it's clear that the implementation should pass every root device to the device selector, and the selector ranks each one.

This queue constructor is the only place where the set of valid device is limited by a context that the user passes in. It could make sense to redefine the spec to say that the implementation only passes devices from the context to the selector in this case. However, that is not how the spec is worded now, and we have the same wording in SYCL 1.2.1.

I actually wonder if this queue constructor is even useful. If the user is passing a custom context, maybe we should also force them to pass a specific device (and not a device selector). This avoids any ambiguity about which devices the implementation passes to the selector. We already have a form of the queue constructor like this (taking a context and a device). If we were doing this from scratch, I think I would not have added a form of the queue constructor that takes a context and a selector.

My feeling is that DPC++ should implement the existing spec, and maybe the committee should consider deprecating this form of the queue constructor.

keryell · 2022-08-02T23:31:16Z

sycl/include/sycl/queue.hpp

+  // see [filter_selectors not "Callable"] in device_selectors.cpp
+  explicit queue(const DeviceSelector &deviceSelector,
+                 const property_list &PropList = {})
+      : queue(detail::select_device(deviceSelector), async_handler{},


keryell · 2022-08-02T23:34:41Z

sycl/source/device_selector.cpp

+              << "SYCL_PI_TRACE[all]: "
+              << "  platform: " << PlatformName << std::endl
+              << "SYCL_PI_TRACE[all]: "
+              << "  device: " << DeviceName << std::endl;


Not clear why you need these strings to be split.

it's copy/pasta. This is the trace code we had before. I just wrapped it in a function, but preserved it otherwise.

keryell · 2022-08-02T23:38:14Z

sycl/source/device_selector.cpp

-                << "  device: " << DeviceName << std::endl;
-    }
+  for (const auto &dev : Devices) {
+    int dev_score = std::invoke(DeviceSelectorInvocable, dev);


Suggested change

int dev_score = std::invoke(DeviceSelectorInvocable, dev);

int dev_score = DeviceSelectorInvocable(dev);

?

@keryell Since we are using std::is_invocable_r_v when we screen it, I actually prefer the symmetry of using std::invoke to invoke it. Maybe I'm in the minority on that. Would you mind if we kept it as is?

Are you kidding me or are you a member of the Church of the C++ Discouragement and Obfuscation? :-)
Why not rewriting all the code with Operator function objects from https://en.cppreference.com/w/cpp/utility/functional instead of using just + or -? ;-)

I don't want to start a holy war. I'll change it.
The hallowed symmetrical beauty of std::is_invocable to std::invoke is perhaps not cherished by the unenlightened.

sycl/source/device_selector.cpp

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

…ss tests AND not have merge conflicts.

cperkinsintel · 2022-08-04T06:15:56Z

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

sycl/include/sycl/queue.hpp

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

…ps://github.com/cperkinsintel/llvm into cperkins-sycl2020-callable-device-selectors--02

pvchupin · 2022-08-05T21:38:50Z

@cperkinsintel, shared libs build fails in post-commit: https://github.com/intel/llvm/runs/7698490682?check_suite_focus=true

intel#6486 added new queue constructors for the use of callable device selectors. In accordance with SYCL 2020 these constructors are marked explicit and as such implicit conversion from initializer lists does not work. These changes fixes the Wait.cpp unittest to use the explicit constructor. Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2022-08-08T10:39:48Z

#6540 should fix the build issue.

#6486 added new queue constructors for the use of callable device selectors. In accordance with SYCL 2020 these constructors are marked explicit and as such implicit conversion from initializer lists does not work. These changes fixes the Wait.cpp unittest to use the explicit constructor. Signed-off-by: Larsen, Steffen <[email protected]>

cperkinsintel added 11 commits July 21, 2022 10:08

preliminary checkpoint

c7a5445

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

clang-format

bf19986

initial pass bringing new callable device selectors to queues. Prefer…

fc971b1

…redDevices in scoring and pi-trace disbled. Signed-off-by: Chris Perkins <[email protected]>

check-sycl passing

668bc6c

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

scoring and trace restored

924d517

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

tracing oversight

9055f14

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

more queue constructors, and some accidental work on platform

8db9516

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

getDevices() - need to keep incomplete types out of headers.

0f30de4

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

inclusion of device_selector.hpp by platform.hpp is choking on device…

6e2b12b

… return type (as opposed to devicePtr). Signed-off-by: Chris Perkins <[email protected]>

platform support. required refactoring to support incomplete types (d…

6ca45b7

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

device constructor and ABI symbols updated

74d76a4

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

cperkinsintel mentioned this pull request Jul 28, 2022

[SYCL] new tests for SYCL 2020 callable device selector interfaces intel/llvm-test-suite#1119

Merged

cperkinsintel added 4 commits July 27, 2022 18:54

windows ABI .dump file

0fb9aca

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

resolve merge conflicts

fca2bc9

windows dump again

a46c090

removed redundant code. should be ready now

53eece9

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

cperkinsintel marked this pull request as ready for review July 28, 2022 04:05

cperkinsintel requested a review from a team as a code owner July 28, 2022 04:05

cperkinsintel requested a review from againull July 28, 2022 04:05

filter_selector is not purely callable. excluding it.

0d2a3ee

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

minor nits

fe70e49

cperkinsintel requested review from romanovvlad and steffenlarsen July 28, 2022 22:37

againull reviewed Jul 29, 2022

View reviewed changes

sycl/test/abi/sycl_symbols_windows.dump Outdated Show resolved Hide resolved

againull reviewed Jul 29, 2022

View reviewed changes

cperkinsintel added 3 commits August 1, 2022 15:12

reviewer feedback and checkpoint

c351206

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

reviewer feedback

6f7e959

overlooked

abc724a

cperkinsintel added 2 commits August 2, 2022 16:01

once again, attempt at windows.dump file

a5237b1

who doesn't love windows.dump

b7270aa

keryell reviewed Aug 2, 2022

View reviewed changes

cperkinsintel added 7 commits August 3, 2022 09:21

reviewer feedback. checkpoint

a351be7

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

overlooked const. linux symbols dump.

f4ce68f

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

windows abi .dump updated

b667aa9

review feedback, alias of enable-if-invocable

2e96c5f

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

windows dump (with feeling)

28dbc1d

merge conflict

0870d34

for the love of all that's holy, please both have the windows.dump pa…

978ac6d

…ss tests AND not have merge conflicts.

keryell reviewed Aug 4, 2022

View reviewed changes

sycl/include/sycl/queue.hpp Outdated Show resolved Hide resolved

too fancy

c2c73ab

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

againull self-requested a review August 5, 2022 05:05

againull previously approved these changes Aug 5, 2022

View reviewed changes

cperkinsintel added 2 commits August 5, 2022 10:26

Merge branch 'sycl' into cperkins-sycl2020-callable-device-selectors--02

c3b042b

Merge branch 'sycl' into cperkins-sycl2020-callable-device-selectors--02

0e1a2fd

cperkinsintel dismissed againull’s stale review via 0e1a2fd August 5, 2022 17:28

cperkinsintel added 2 commits August 5, 2022 10:30

Merge branch 'cperkins-sycl2020-callable-device-selectors--02' of htt…

c043715

…ps://github.com/cperkinsintel/llvm into cperkins-sycl2020-callable-device-selectors--02

merge conflict readdressed to appease clang-format

c54ea3d

againull self-requested a review August 5, 2022 20:29

againull approved these changes Aug 5, 2022

View reviewed changes

againull merged commit 64f0db7 into intel:sycl Aug 5, 2022

steffenlarsen mentioned this pull request Aug 8, 2022

[SYCL] Fix unittest initialization of queue with selector #6540

Merged

hdelan mentioned this pull request Aug 29, 2022

Add sycl quick refs codeplaysoftware/syclacademy#179

Merged


		} // namespace detail

		device device_selector::select_device() const {

	: queue(detail::select_device(deviceSelector), AsyncHandler, PropList) {}
	: queue(device { deviceSelector }, AsyncHandler, PropList) {}

	int dev_score = std::invoke(DeviceSelectorInvocable, dev);
	int dev_score = DeviceSelectorInvocable(dev);

[SYCL] SYCL 2020 callable device selectors #6486

[SYCL] SYCL 2020 callable device selectors #6486

Uh oh!

Conversation

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

Uh oh!

Uh oh!

cperkinsintel commented Jul 28, 2022

Uh oh!

cperkinsintel commented Jul 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cperkinsintel Aug 3, 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!

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!

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!

Uh oh!

cperkinsintel commented Aug 4, 2022

Uh oh!

Uh oh!

pvchupin commented Aug 5, 2022

Uh oh!

steffenlarsen commented Aug 8, 2022

Uh oh!

Uh oh!

cperkinsintel commented Jul 28, 2022 •

edited

Loading

cperkinsintel Aug 3, 2022 •

edited

Loading