Skip to content

[SYCL][Graph] Fix issue when updating HostTasks with whole graph update #17645

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
merged 2 commits into from
Apr 3, 2025

Conversation

fabiomestre
Copy link
Contributor

@fabiomestre fabiomestre commented Mar 25, 2025

When updating a HostTask using whole graph update, we rely
on the Sycl Scheduler to execute the HostTask and all its
dependencies. The Scheduler executes this work asynchronously
using a separate thread. Since exec_graph.update() was non-blocking,
this was causing issues, where the user code could, for example,
free resources, before the update() command (which is a dependent on the
HostTask completion) could execute.

This commit fixes this issue by making exec_graph.update() blocking
when host_tasks are used.

@fabiomestre fabiomestre force-pushed the fabio/fix_host_task_whole_graph_update branch from 030bb18 to 2d8d5e8 Compare March 26, 2025 12:46
@fabiomestre fabiomestre force-pushed the fabio/fix_host_task_whole_graph_update branch from 2d8d5e8 to 1a70572 Compare March 27, 2025 13:43
@fabiomestre fabiomestre changed the title :Fix host task whole graph update bug [Sycl][Graph] Fix issue when updating HostTasks with whole graph update Mar 27, 2025
@fabiomestre fabiomestre marked this pull request as ready for review March 27, 2025 13:44
@fabiomestre fabiomestre requested a review from a team as a code owner March 27, 2025 13:44
Copy link
Contributor

@konradkusiak97 konradkusiak97 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Bensuo Bensuo left a comment

Choose a reason for hiding this comment

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

LGTM!

When updating a HostTask using whole graph update, we rely
on the Sycl Scheduler to execute the HostTask and all its
dependencies. The Scheduler executes this work asynchronously
using a separate thread. Since exec_graph.update() was non-blocking,
this was causing issues, where the user code could, for example,
free resources, before the update() command (which is a dependent on the
HostTask completion) could execute.

This commit fixes this issue by making exec_graph.update() blocking
when host_tasks are used.
@fabiomestre fabiomestre force-pushed the fabio/fix_host_task_whole_graph_update branch from 1a70572 to 95a6582 Compare April 1, 2025 16:21
@EwanC EwanC temporarily deployed to WindowsCILock April 3, 2025 06:47 — with GitHub Actions Inactive
@EwanC EwanC changed the title [Sycl][Graph] Fix issue when updating HostTasks with whole graph update [SYCL][Graph] Fix issue when updating HostTasks with whole graph update Apr 3, 2025
@EwanC EwanC temporarily deployed to WindowsCILock April 3, 2025 07:21 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock April 3, 2025 07:21 — with GitHub Actions Inactive
@EwanC
Copy link
Contributor

EwanC commented Apr 3, 2025

@intel/llvm-gatekeepers This is ready to merge thanks

@martygrant martygrant merged commit 52c2bc9 into intel:sycl Apr 3, 2025
23 checks passed
ggojska pushed a commit to ggojska/llvm that referenced this pull request Apr 7, 2025
…te (intel#17645)

When updating a HostTask using whole graph update, we rely
on the Sycl Scheduler to execute the HostTask and all its
dependencies. The Scheduler executes this work asynchronously
using a separate thread. Since exec_graph.update() was non-blocking,
this was causing issues, where the user code could, for example,
free resources, before the update() command (which is a dependent on the
HostTask completion) could execute.

This commit fixes this issue by making exec_graph.update() blocking
when host_tasks are used.

Co-authored-by: Ewan Crawford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants