Skip to content
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

[SYCL] Add improved host task extension #13339

Open
wants to merge 1 commit into
base: sycl
Choose a base branch
from

Conversation

AerialMantis
Copy link
Contributor

@AerialMantis AerialMantis commented Apr 9, 2024

Introduce a new extension which extends the host task interfaces to allow host task functions to enqueue asynchronous backend-specific commands and connect these commands with the SYCL DAG.

This is done in two parts:

Firstly to introduce an interface; handler::get_native_events, which is enabled by a property; manual_interop_sync, which allows users to retrieve native events which would ordinarily be synchronized with before the host task function is executed, having these instead handled by the the host task function, as dependencies to an asynchronous backend API.

Secondly to introduce an interface; handler::add_native_events which allows users to add additional native events which are produced from an asynchronous backend API, to then be encapsulated in the SYCL event returned by submit.

This extension is implemented in #12921

Introduce a new extension which extends the host task interfaces to
allow host task functions to enqueue asynchronous backend-specific
commands and connect these commands with the SYCL DAG.

This is done in two parts:

Firstly to introduce an interface; `handler::get_native_events`, which
is enabled by a property; `manual_interop_sync`, which allows users to
retrieve native events which would ordinarily be synchronized with
before the host task function is executed, having these instead handled
by the the host task function, as dependencies to an asynchronous
backend API.

Secondly to introduce an interface; `handler::add_native_events` which
allows users to add additional native events which are produced from an
asynchronous backend API, to then be encapsulated in the SYCL event
returned by `submit`.
events to be passed to the host task function instead, allowing the host task
function to be invoked without requiring full completion of said incoming
events. This extension also introduces an interface which allows those native
vents to be retrieved from within the host task function.
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
vents to be retrieved from within the host task function.
events to be retrieved from within the host task function.

----

_Effects_: Returns a `std::vector` of the native events for the backend
`Backend` for the dependencies for the dependencies to the host task command if
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
`Backend` for the dependencies for the dependencies to the host task command if
`Backend` for the dependencies to the host task command if


cl_event ne2;
clEnqueueReadBuffer(queue, mem, CL_FALSE, 0, sizeof(int), &pattern, 1,
&nativeEvent1, &ne2);
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
&nativeEvent1, &ne2);
&ne1, &ne2);

to an OpenCL command queue asynchronously. The OpenCL event which results from
enqueueing these commands is then converted to a SYCL `event` via the backend
interop interface. Then the created SYCL `event` is passed to the host task via
`interop_handle::ext_oneapi_add_event`.
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
`interop_handle::ext_oneapi_add_event`.
`interop_handle::ext_oneapi_add_native_events`.

[source,c++]
----
template <backend Backend>
std::vector<backend_return_t<Backend, event>> ext_oneapi_get_native_events();
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
std::vector<backend_return_t<Backend, event>> ext_oneapi_get_native_events();
std::vector<backend_return_t<Backend, native_event>> ext_oneapi_get_native_events();

event will give people the wrong assumption that it is sycl::event

----
template <backend Backend>
void ext_oneapi_add_native_events(
backend_return_t<Backend, event> hostTaskEvent);
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
backend_return_t<Backend, event> hostTaskEvent);
backend_return_t<Backend, native_event> hostTaskEvent);


template <backend Backend>
void ext_oneapi_add_native_events(
const std::vector<backend_return_t<Backend, event>> &hostTaskEvents)
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
const std::vector<backend_return_t<Backend, event>> &hostTaskEvents)
const std::vector<backend_return_t<Backend, native_event>> &hostTaskEvents)


As the SYCL `event` that is returned from the submission of the host task is
created before the host task function is executed, it is necessary for the
SYCL `event`(s) passed to `ext_oneapi_add_native_events` be stored in a place
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
SYCL `event`(s) passed to `ext_oneapi_add_native_events` be stored in a place
native event(s) passed to `ext_oneapi_add_native_events` be stored in a place

created before the host task function is executed, it is necessary for the
SYCL `event`(s) passed to `ext_oneapi_add_native_events` be stored in a place
accessible to the `event`, and access to this location must be provided to the
`interop_handle` so that SYCL `events` added from the host task function can be
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
`interop_handle` so that SYCL `events` added from the host task function can be
`interop_handle` so that native events added from the host task function can be


cgh.host_task([&](sycl::interop_handle &ih, {ext::oneapi::experimental}) {
// Dependent events are returned to be synchronized with.
auto nativeEvents = ih.get_native_events<backend::opencl>();
Copy link
Contributor

Choose a reason for hiding this comment

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

where do nativeEvents come from, from line 178 ih.ext_oneapi_add_native_events<backend::opencl>(ne2); of the last host_task?

If the answer is yes, how about host_task1 --> sycl kernel --> host_task2, actually, the host_task2 does not depend on the host_task1, but depends on sycl kernel, so, we don't need the waiting events in line 270, the dependency is handled in line 256.

Even for the case host_task1 --> host_task2, since the native event in host_task1 is wrapped as sycl::event, and so for host_task2, line 256 is enough, we don't need the waiting events at line 270.

Looks that get_native_events is not necessary.


// The event returned by the host task function are waiting on by the event
// returned by submit
ih.ext_oneapi_add_native_events<backend::opencl>(ne2);
Copy link
Contributor

Choose a reason for hiding this comment

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

for a corner case that there are several native events generated and they are independent.
ih.ext_oneapi_add_native_eventsbackend::opencl(nes);

Is it possible for e2 to represent all the native events in nes? My understanding is that e2 is just a wrapper of one native event.

Comment on lines +59 to +60
includes both asynchronous commands executing before the the host task which the
host task depends on, and asynchornous commands executing after the host task
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
includes both asynchronous commands executing before the the host task which the
host task depends on, and asynchornous commands executing after the host task
includes both asynchronous commands executing before the host task which the
host task depends on, and asynchronous commands executing after the host task

Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants