-
Notifications
You must be signed in to change notification settings - Fork 788
[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
Changes from all commits
c7a5445
bf19986
fc971b1
668bc6c
924d517
9055f14
8db9516
0f30de4
6e2b12b
6ca45b7
74d76a4
0fb9aca
fca2bc9
a46c090
53eece9
0d2a3ee
fe70e49
c351206
6f7e959
abc724a
a5237b1
b7270aa
a351be7
f4ce68f
b667aa9
2e96c5f
28dbc1d
0870d34
978ac6d
c2c73ab
c3b042b
0e1a2fd
c043715
c54ea3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. | ||||||
|
@@ -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) {} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting you need this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting feature, but this is not how the SYCL specification is defined:
So either the specification has to be changed or DPC++ has to be updated. @bader any direction? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{}, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = {}) | ||||||
|
@@ -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, | ||||||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.