-
Notifications
You must be signed in to change notification settings - Fork 802
[SYCL][L0] Adds device member to L0 make_queue input type #6148
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
5dbf6d4
955d58e
8e59b1f
eaa8cc6
93fac38
39f6d37
28d39e1
5174989
83f8652
4ccd441
54cfe32
c269e64
fe0b0b4
896f8ef
1c2aa9e
c051a32
f578f39
63232c9
7e00bf8
f61144b
2ee7a0a
2554074
e0e9b13
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 |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ This extension provides a feature-test macro as described in the core SYCL speci | |
| |---|:---| | ||
| |1|Initial extension version. | ||
| |2|Added support for the make_buffer() API. | ||
| |3|Added device member to backend_input_t<backend::ext_oneapi_level_zero, queue>. | ||
|
|
||
| NOTE: This extension is following SYCL 2020 backend specification. Prior API for interoperability with Level-Zero is marked | ||
| as deprecated and will be removed in the next release. | ||
|
|
@@ -108,8 +109,8 @@ struct { | |
| ``` | ||
| </td> | ||
| </tr><tr> | ||
| <td>queue</td> | ||
| <td><pre>ze_command_queue_handle_t</pre></td> | ||
| <td rowspan="2">queue</td> | ||
| <td rowspan="2"><pre>ze_command_queue_handle_t</pre></td> | ||
| <td> | ||
|
|
||
| ``` C++ | ||
|
|
@@ -119,6 +120,22 @@ struct { | |
| ext::oneapi::level_zero::ownership::transfer}; | ||
| } | ||
| ``` | ||
|
|
||
| Deprecated as of version 3 of this specification.[^1] | ||
| </td> | ||
| </tr><tr> | ||
| <td> | ||
|
|
||
| ``` C++ | ||
| struct { | ||
| ze_command_queue_handle_t NativeHandle; | ||
| device Device; | ||
|
Contributor
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. The commit message indicates that prior to this PR the device associated with the constructed queue is unspecified. However, the Level Zero interop spec does specify which device is associated with the queue:
Was the code implemented to do that before? Maybe we don't need this PR? Though, I do agree that passing a specific device is more friendly.
Contributor
Author
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 bad. Yes, it seems to be in line with the old behavior and I will need to make a small adjustment to recover this. That said, this seems to have been a mistake in the design. The user (and the aforementioned test) could be required to make sure the context only has the right device, but this seems too restrictive and it could potentially cause problems if the native context was actually created with multiple devices. |
||
| ext::oneapi::level_zero::ownership Ownership{ | ||
| ext::oneapi::level_zero::ownership::transfer}; | ||
| } | ||
| ``` | ||
|
|
||
| Supported since version 3 of this specification.[^1] | ||
| </td> | ||
| </tr><tr> | ||
| <td>event</td> | ||
|
|
@@ -191,6 +208,8 @@ struct { | |
| </tr> | ||
| </table> | ||
|
|
||
| [^1]: The SYCL implementation is responsible for distinguishing between the variants of <code>backend_input_t<backend::ext_oneapi_level_zero, queue></code>. | ||
|
|
||
| ### 4.2 Obtaining of native Level-Zero handles from SYCL objects | ||
|
|
||
| The ```sycl::get_native<backend::ext_oneapi_level_zero>``` free-function is how a raw native Level-Zero handle can be obtained | ||
|
|
@@ -275,7 +294,10 @@ make_queue<backend::ext_oneapi_level_zero>( | |
| const context &Context) | ||
| ``` | ||
| </td> | ||
| <td>Constructs a SYCL queue instance from a Level-Zero <code>ze_command_queue_handle_t</code>. The <code>Context</code> argument must be a valid SYCL context encapsulating a Level-Zero context. The queue is attached to the first device in the passed SYCL context. The <code>Ownership</code> input structure member specifies if the SYCL runtime should take ownership of the passed native handle. The default behavior is to transfer the ownership to the SYCL runtime. See section 4.4 for details.</td> | ||
| <td>Constructs a SYCL queue instance from a Level-Zero <code>ze_command_queue_handle_t</code>. The <code>Context</code> argument must be a valid SYCL context encapsulating a Level-Zero context. The <code>Device</code> input structure member specifies the device to create the <code>queue</code> against and must be in <code>Context</code>. The <code>Ownership</code> input structure member specifies if the SYCL runtime should take ownership of the passed native handle. The default behavior is to transfer the ownership to the SYCL runtime. See section 4.4 for details. | ||
|
|
||
| If the deprecated variant of <code>backend_input_t<backend::ext_oneapi_level_zero, queue></code> is passed to <code>make_queue</code> the queue is attached to the first device in <code>Context</code>. | ||
| </td> | ||
| </tr><tr> | ||
| <td> | ||
|
|
||
|
|
@@ -485,3 +507,4 @@ struct free_memory { | |
| |6|2021-08-30|Dmitry Vodopyanov|Updated according to SYCL 2020 reqs for extensions | ||
| |7|2021-09-13|Sergey Maslov|Updated according to SYCL 2020 standard | ||
| |8|2022-01-06|Artur Gainullin|Introduced make_buffer() API | ||
| |9|2022-05-12|Steffen Larsen|Added device member to queue input type | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| #include <CL/sycl/kernel_bundle.hpp> | ||
| #include <CL/sycl/queue.hpp> | ||
| #include <sycl/ext/oneapi/backend/level_zero_ownership.hpp> | ||
| #include <sycl/ext/oneapi/filter_selector.hpp> | ||
|
|
||
| typedef struct _ze_command_queue_handle_t *ze_command_queue_handle_t; | ||
| typedef struct _ze_context_handle_t *ze_context_handle_t; | ||
|
|
@@ -38,6 +39,9 @@ __SYCL_INLINE_NAMESPACE(cl) { | |
| namespace sycl { | ||
| namespace detail { | ||
|
|
||
| // Forward declarations | ||
| class device_impl; | ||
|
|
||
| // TODO the interops for context, device, event, platform and program | ||
| // may be removed after removing the deprecated 'get_native()' methods | ||
| // from the corresponding classes. The interop<backend, queue> specialization | ||
|
|
@@ -130,11 +134,61 @@ template <> struct BackendReturn<backend::ext_oneapi_level_zero, event> { | |
| using type = ze_event_handle_t; | ||
| }; | ||
|
|
||
| struct OptionalDevice { | ||
smaslov-intel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| OptionalDevice() : DeviceImpl(nullptr) {} | ||
| OptionalDevice(device dev) : DeviceImpl(getSyclObjImpl(dev)) {} | ||
|
|
||
|
Contributor
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 think we need an assignment operator here too.
Contributor
Author
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. Good point. Assignment operators have been added.
Contributor
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. Sorry, I meant that we need an assignment operator to
Contributor
Author
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. Sorry, that was my bad. It has been amended. |
||
| operator device() const { | ||
| if (!DeviceImpl) | ||
| throw runtime_error("No device has been set.", PI_INVALID_DEVICE); | ||
| return createSyclObjFromImpl<device>(DeviceImpl); | ||
| } | ||
|
|
||
| OptionalDevice &operator=(OptionalDevice &Other) { | ||
| DeviceImpl = Other.DeviceImpl; | ||
| return *this; | ||
| } | ||
| OptionalDevice &operator=(device &Other) { | ||
| DeviceImpl = getSyclObjImpl(Other); | ||
| return *this; | ||
| } | ||
|
|
||
| private: | ||
| std::shared_ptr<device_impl> DeviceImpl; | ||
|
|
||
| friend bool OptionalDeviceHasDevice(const OptionalDevice &Dev); | ||
| }; | ||
|
|
||
| // Inspector function in the detail namespace to avoid exposing | ||
| // OptionalDevice::hasDevice to user-space. | ||
| inline bool OptionalDeviceHasDevice(const OptionalDevice &Dev) { | ||
| return Dev.DeviceImpl != nullptr; | ||
| } | ||
|
|
||
| template <> struct BackendInput<backend::ext_oneapi_level_zero, queue> { | ||
| struct type { | ||
| interop<backend::ext_oneapi_level_zero, queue>::type NativeHandle; | ||
| ext::oneapi::level_zero::ownership Ownership{ | ||
| ext::oneapi::level_zero::ownership::transfer}; | ||
| ext::oneapi::level_zero::ownership Ownership; | ||
|
|
||
| // TODO: Change this to be device when the deprecated constructor is | ||
| // removed. | ||
| OptionalDevice Device; | ||
|
|
||
| type() | ||
| : Ownership(ext::oneapi::level_zero::ownership::transfer), Device() {} | ||
|
|
||
| __SYCL_DEPRECATED("Use backend_input_t<backend::ext_oneapi_level_zero, " | ||
| "queue> constructor with device parameter") | ||
| type(interop<backend::ext_oneapi_level_zero, queue>::type nativeHandle, | ||
| ext::oneapi::level_zero::ownership ownership = | ||
| ext::oneapi::level_zero::ownership::transfer) | ||
| : NativeHandle(nativeHandle), Ownership(ownership), Device() {} | ||
|
|
||
| type(interop<backend::ext_oneapi_level_zero, queue>::type nativeHandle, | ||
| device dev, | ||
| ext::oneapi::level_zero::ownership ownership = | ||
| ext::oneapi::level_zero::ownership::transfer) | ||
| : NativeHandle(nativeHandle), Ownership(ownership), Device(dev) {} | ||
| }; | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,10 +44,11 @@ | |
| // piQueueFlush function. | ||
| // 7.9 Added new context and ownership arguments to | ||
| // piextMemCreateWithNativeHandle. | ||
| // 8.10 Added new optional device argument to piextQueueCreateWithNativeHandle | ||
| // | ||
| #include "CL/cl.h" | ||
| #define _PI_H_VERSION_MAJOR 7 | ||
| #define _PI_H_VERSION_MINOR 9 | ||
| #define _PI_H_VERSION_MAJOR 8 | ||
| #define _PI_H_VERSION_MINOR 10 | ||
|
|
||
| #define _PI_STRING_HELPER(a) #a | ||
| #define _PI_CONCAT(a, b) _PI_STRING_HELPER(a.b) | ||
|
|
@@ -1157,12 +1158,15 @@ piextQueueGetNativeHandle(pi_queue queue, pi_native_handle *nativeHandle); | |
| /// | ||
| /// \param nativeHandle is the native handle to create PI queue from. | ||
| /// \param context is the PI context of the queue. | ||
| /// \param queue is the PI queue created from the native handle. | ||
| /// \param device is the PI device associated with the native device used when | ||
| /// creating the native queue. This parameter is optional but some backends | ||
smaslov-intel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// may fail to create the right PI queue if omitted. | ||
| /// \param pluginOwnsNativeHandle Indicates whether the created PI object | ||
| /// should take ownership of the native handle. | ||
| /// \param queue is the PI queue created from the native handle. | ||
| __SYCL_EXPORT pi_result piextQueueCreateWithNativeHandle( | ||
| pi_native_handle nativeHandle, pi_context context, pi_queue *queue, | ||
| bool pluginOwnsNativeHandle); | ||
| pi_native_handle nativeHandle, pi_context context, pi_device device, | ||
| bool pluginOwnsNativeHandle, pi_queue *queue); | ||
|
|
||
| // | ||
| // Memory | ||
|
|
@@ -1819,9 +1823,9 @@ struct _pi_plugin { | |
| // Some choices are: | ||
| // - Use of integers to keep major and minor version. | ||
| // - Keeping char* Versions. | ||
| char PiVersion[4]; | ||
| char PiVersion[10]; | ||
|
Contributor
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. Let's make this size into a "const int" parameter, and stop needing to sync all users.
Contributor
Author
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. Some backends simply use |
||
| // Plugin edits this. | ||
| char PluginVersion[4]; | ||
| char PluginVersion[10]; | ||
| char *Targets; | ||
| struct FunctionPointers { | ||
| #define _PI_API(api) decltype(::api) *api; | ||
|
|
||
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.
Do we need to mention that the old variant is still usable, although deprecated? Maybe it is covered by:
We could include the constructors, but they are really only there to allow
Deviceto be beforeOwnershipas it is more in line with other input types, like the one forcontext. I fear including the constructors here will only make it less readable.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.
Yes, I think we should document the deprecated API. Since this PR changes the API, the feature-test macro should also be incremented to
3, and the new form ofmake_queueshould note that it was added in version3of this extension.I don't understand your comment about the constructor. Are you referring to the
make_queuefunction as the "constructor"?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.
By constructor, I mean the constructor of the struct the new
Devicemember. It is unnamed in this document, but in the implementation we haveThis allows both
make_queue(... {ZeCommandQueueHandle, MyOwnership} ...)andmake_queue(... {ZeCommandQueueHandle, MyDevice, MyOwnership} ...), where the former is deprecated.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.
What if someone has existing code like this: