Skip to content

[SYCL] SYCL 2020 callable device selectors #6486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
c7a5445
preliminary checkpoint
cperkinsintel Jul 21, 2022
bf19986
clang-format
cperkinsintel Jul 21, 2022
fc971b1
initial pass bringing new callable device selectors to queues. Prefe…
cperkinsintel Jul 22, 2022
668bc6c
check-sycl passing
cperkinsintel Jul 25, 2022
924d517
scoring and trace restored
cperkinsintel Jul 26, 2022
9055f14
tracing oversight
cperkinsintel Jul 26, 2022
8db9516
more queue constructors, and some accidental work on platform
cperkinsintel Jul 26, 2022
0f30de4
getDevices() - need to keep incomplete types out of headers.
cperkinsintel Jul 26, 2022
6e2b12b
inclusion of device_selector.hpp by platform.hpp is choking on device…
cperkinsintel Jul 26, 2022
6ca45b7
platform support. required refactoring to support incomplete types (d…
cperkinsintel Jul 27, 2022
74d76a4
device constructor and ABI symbols updated
cperkinsintel Jul 28, 2022
0fb9aca
windows ABI .dump file
cperkinsintel Jul 28, 2022
fca2bc9
resolve merge conflicts
cperkinsintel Jul 28, 2022
a46c090
windows dump again
cperkinsintel Jul 28, 2022
53eece9
removed redundant code. should be ready now
cperkinsintel Jul 28, 2022
0d2a3ee
filter_selector is not purely callable. excluding it.
cperkinsintel Jul 28, 2022
fe70e49
minor nits
cperkinsintel Jul 28, 2022
c351206
reviewer feedback and checkpoint
cperkinsintel Aug 1, 2022
6f7e959
reviewer feedback
cperkinsintel Aug 2, 2022
abc724a
overlooked
cperkinsintel Aug 2, 2022
a5237b1
once again, attempt at windows.dump file
cperkinsintel Aug 2, 2022
b7270aa
who doesn't love windows.dump
cperkinsintel Aug 2, 2022
a351be7
reviewer feedback. checkpoint
cperkinsintel Aug 3, 2022
f4ce68f
overlooked const. linux symbols dump.
cperkinsintel Aug 3, 2022
b667aa9
windows abi .dump updated
cperkinsintel Aug 3, 2022
2e96c5f
review feedback, alias of enable-if-invocable
cperkinsintel Aug 4, 2022
28dbc1d
windows dump (with feeling)
cperkinsintel Aug 4, 2022
0870d34
merge conflict
cperkinsintel Aug 4, 2022
978ac6d
for the love of all that's holy, please both have the windows.dump pa…
cperkinsintel Aug 4, 2022
c2c73ab
too fancy
cperkinsintel Aug 5, 2022
c3b042b
Merge branch 'sycl' into cperkins-sycl2020-callable-device-selectors--02
cperkinsintel Aug 5, 2022
0e1a2fd
Merge branch 'sycl' into cperkins-sycl2020-callable-device-selectors--02
cperkinsintel Aug 5, 2022
c043715
Merge branch 'cperkins-sycl2020-callable-device-selectors--02' of htt…
cperkinsintel Aug 5, 2022
c54ea3d
merge conflict readdressed to appease clang-format
cperkinsintel Aug 5, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion sycl/include/sycl/device.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ class device_impl;
auto getDeviceComparisonLambda();
} // namespace detail

namespace ext {
namespace oneapi {
// Forward declaration
class filter_selector;
} // namespace oneapi
} // namespace ext

/// The SYCL device class encapsulates a single SYCL device on which kernels
/// may be executed.
///
Expand All @@ -53,9 +60,20 @@ class __SYCL_EXPORT device {
/// Constructs a SYCL device instance using the device selected
/// by the DeviceSelector provided.
///
/// \param DeviceSelector SYCL device selector to be used (see 4.6.1.1).
/// \param DeviceSelector SYCL 1.2.1 device_selector to be used (see 4.6.1.1).
explicit device(const device_selector &DeviceSelector);

#if __cplusplus >= 201703L
/// Constructs a SYCL device instance using the device
/// identified by the device selector provided.
/// \param DeviceSelector is SYCL 2020 Device Selector, a simple callable that
/// takes a device and returns an int
template <typename DeviceSelector,
typename = detail::EnableIfDeviceSelectorInvocable<DeviceSelector>>
explicit device(const DeviceSelector &deviceSelector)
: device(detail::select_device(deviceSelector)) {}
#endif

bool operator==(const device &rhs) const { return impl == rhs.impl; }

bool operator!=(const device &rhs) const { return !(*this == rhs); }
Expand Down
41 changes: 36 additions & 5 deletions sycl/include/sycl/device_selector.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,19 @@ namespace sycl {
// Forward declarations
class device;

/// The device_selector class provides ability to choose the best SYCL device
/// based on heuristics specified by the user.
namespace ext {
namespace oneapi {
class filter_selector;
}
} // namespace ext

/// The SYCL 1.2.1 device_selector class provides ability to choose the
/// best SYCL device based on heuristics specified by the user.
///
/// \sa device
///
/// \ingroup sycl_api_dev_sel
class __SYCL_EXPORT device_selector {
protected:
// SYCL 1.2.1 defines a negative score to reject a device from selection
static constexpr int REJECT_DEVICE_SCORE = -1;

public:
virtual ~device_selector() = default;
Expand Down Expand Up @@ -87,5 +90,33 @@ class __SYCL_EXPORT host_selector : public device_selector {
public:
int operator()(const device &dev) const override;
};

namespace detail {

// SYCL 2020 section 4.6.1.1 defines a negative score to reject a device from
// selection
static constexpr int REJECT_DEVICE_SCORE = -1;

using DSelectorInvocableType = std::function<int(const sycl::device &)>;

#if __cplusplus >= 201703L

// Enable if DeviceSelector callable has matching signature, but
// exclude if descended from filter_selector which is not purely callable.
// See [FilterSelector not Callable] in device_selector.cpp
template <typename DeviceSelector>
using EnableIfDeviceSelectorInvocable = std::enable_if_t<
std::is_invocable_r_v<int, DeviceSelector &, const device &> &&
!std::is_base_of_v<ext::oneapi::filter_selector, DeviceSelector>>;
#endif

__SYCL_EXPORT device
select_device(const DSelectorInvocableType &DeviceSelectorInvocable);

__SYCL_EXPORT device
select_device(const DSelectorInvocableType &DeviceSelectorInvocable,
const context &SyclContext);

} // namespace detail
} // namespace sycl
} // __SYCL_INLINE_NAMESPACE(cl)
24 changes: 22 additions & 2 deletions sycl/include/sycl/platform.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <sycl/detail/common.hpp>
#include <sycl/detail/export.hpp>
#include <sycl/detail/info_desc_helpers.hpp>
#include <sycl/device_selector.hpp>
#include <sycl/stl.hpp>

// 4.6.2 Platform class
Expand All @@ -31,6 +32,12 @@ auto get_native(const SyclObjectT &Obj)
namespace detail {
class platform_impl;
}
namespace ext {
namespace oneapi {
// Forward declaration
class filter_selector;
} // namespace oneapi
} // namespace ext

/// Encapsulates a SYCL platform on which kernels may be executed.
///
Expand All @@ -50,15 +57,26 @@ class __SYCL_EXPORT platform {
explicit platform(cl_platform_id PlatformId);
#endif

/// Constructs a SYCL platform instance using device selector.
/// Constructs a SYCL platform instance using a device_selector.
///
/// One of the SYCL devices that is associated with the constructed SYCL
/// platform instance must be the SYCL device that is produced from the
/// provided device selector.
///
/// \param DeviceSelector is an instance of SYCL device_selector.
/// \param DeviceSelector is an instance of a SYCL 1.2.1 device_selector
explicit platform(const device_selector &DeviceSelector);

#if __cplusplus >= 201703L
/// Constructs a SYCL platform instance using the platform of the device
/// identified by the device selector provided.
/// \param DeviceSelector is SYCL 2020 Device Selector, a simple callable that
/// takes a device and returns an int
template <typename DeviceSelector,
typename = detail::EnableIfDeviceSelectorInvocable<DeviceSelector>>
explicit platform(const DeviceSelector &deviceSelector)
: platform(detail::select_device(deviceSelector)) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this does not work?

Suggested change
: platform(detail::select_device(deviceSelector)) {}
: platform(device { deviceSelector }) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The honest answer is I didn't think of it. But device is an incomplete type here in platform.hpp, so that's the reason it won't work.

#endif

platform(const platform &rhs) = default;

platform(platform &&rhs) = default;
Expand Down Expand Up @@ -141,6 +159,8 @@ class __SYCL_EXPORT platform {
std::shared_ptr<detail::platform_impl> impl;
platform(std::shared_ptr<detail::platform_impl> impl) : impl(impl) {}

platform(const device &Device);

template <class T>
friend T detail::createSyclObjFromImpl(decltype(T::impl) ImplObj);
template <class Obj>
Expand Down
63 changes: 61 additions & 2 deletions sycl/include/sycl/queue.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#define __STDC_FORMAT_MACROS 1
#endif
#include <cinttypes>
#include <type_traits>
#include <utility>

// having _TWO_ mid-param #ifdefs makes the functions very difficult to read.
Expand Down Expand Up @@ -123,10 +124,68 @@ class __SYCL_EXPORT queue {
queue(const async_handler &AsyncHandler, const property_list &PropList = {})
: queue(default_selector(), AsyncHandler, PropList) {}

#if __cplusplus >= 201703L
/// Constructs a SYCL queue instance using the device identified by the
/// device selector provided.
/// \param DeviceSelector is SYCL 2020 Device Selector, a simple callable that
/// takes a device and returns an int
/// \param AsyncHandler is a SYCL asynchronous exception handler.
/// \param PropList is a list of properties for queue construction.
template <typename DeviceSelector,
typename = detail::EnableIfDeviceSelectorInvocable<DeviceSelector>>
explicit queue(const DeviceSelector &deviceSelector,
const async_handler &AsyncHandler,
const property_list &PropList = {})
: queue(detail::select_device(deviceSelector), AsyncHandler, PropList) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

?

Copy link
Contributor Author

@cperkinsintel cperkinsintel Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Constructs a SYCL queue instance using the device identified by the
/// device selector provided.
/// \param DeviceSelector is SYCL 2020 Device Selector, a simple callable that
/// takes a device and returns an int
/// \param PropList is a list of properties for queue construction.
template <typename DeviceSelector,
typename = detail::EnableIfDeviceSelectorInvocable<DeviceSelector>>
explicit queue(const DeviceSelector &deviceSelector,
const property_list &PropList = {})
: queue(detail::select_device(deviceSelector), async_handler{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idem?

PropList) {}

/// Constructs a SYCL queue instance using the device identified by the
/// device selector provided.
/// \param SyclContext is an instance of SYCL context.
/// \param DeviceSelector is SYCL 2020 Device Selector, a simple callable that
/// takes a device and returns an int
/// \param PropList is a list of properties for queue construction.
template <typename DeviceSelector,
typename = detail::EnableIfDeviceSelectorInvocable<DeviceSelector>>
explicit queue(const context &syclContext,
const DeviceSelector &deviceSelector,
const property_list &propList = {})
: queue(syclContext, detail::select_device(deviceSelector, syclContext),
propList) {}

/// Constructs a SYCL queue instance using the device identified by the
/// device selector provided.
/// \param SyclContext is an instance of SYCL context.
/// \param DeviceSelector is SYCL 2020 Device Selector, a simple callable that
/// takes a device and returns an int
/// \param AsyncHandler is a SYCL asynchronous exception handler.
/// \param PropList is a list of properties for queue construction.
template <typename DeviceSelector,
typename = detail::EnableIfDeviceSelectorInvocable<DeviceSelector>>
explicit queue(const context &syclContext,
const DeviceSelector &deviceSelector,
const async_handler &AsyncHandler,
const property_list &propList = {})
: queue(syclContext, detail::select_device(deviceSelector, syclContext),
AsyncHandler, propList) {}

#endif

/// Constructs a SYCL queue instance using the device returned by the
/// DeviceSelector provided.
///
/// \param DeviceSelector is an instance of SYCL device selector.
/// \param DeviceSelector is an instance of a SYCL 1.2.1 device_selector.
/// \param PropList is a list of properties for queue construction.
queue(const device_selector &DeviceSelector,
const property_list &PropList = {})
Expand All @@ -135,7 +194,7 @@ class __SYCL_EXPORT queue {
/// Constructs a SYCL queue instance with an async_handler using the device
/// returned by the DeviceSelector provided.
///
/// \param DeviceSelector is an instance of SYCL device selector.
/// \param DeviceSelector is an instance of SYCL 1.2.1 device_selector.
/// \param AsyncHandler is a SYCL asynchronous exception handler.
/// \param PropList is a list of properties for queue construction.
queue(const device_selector &DeviceSelector,
Expand Down
Loading