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][Graph] in-order queue barrier fix #13193

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Mar 28, 2024

Fix for #13066

The special case for using barriers on an in-order queue is that the last event/node submitted to the queue is used as an event for the barrier to depend on.

Looking at the last command submitted to the queue isn't correct for a graph, because previous commands
submitted to a graph could have been added explicitly or from recording another queue. Therefore, there is not guaranteed that the last command submitted by the in-order queue is correct dependency for the barrier node in the graph.

Fix for intel#13066

The special case for using barriers on an in-order queue
is that the last event/node submitted to the queue is used
as an event for the barrier to depend on.

Looking at the last command submitted to the queue isn't
correct for a graph, because previous commands
submitted to a graph could have been added explicitly or
from recording another queue. Therefore, there is not
guaranteed that the last command submitted by the in-order
queue is correct dependency for the barrier node in the graph.
@EwanC
Copy link
Contributor Author

EwanC commented Mar 29, 2024

The InorderQueue/in_order_ext_oneapi_submit_barrier.cpp E2E test which is failing on HIP does have graph extension code in it https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/InorderQueue/in_order_ext_oneapi_submit_barrier.cpp#L72 So this is a fail that I need to look into (although it isn't yet clear to me why it only affects HIP backends)

Co-authored-by: Ori Sky <4142775+ori-sky@users.noreply.github.com>
@EwanC EwanC marked this pull request as ready for review March 29, 2024 15:37
@EwanC EwanC requested review from a team as code owners March 29, 2024 15:37
@EwanC
Copy link
Contributor Author

EwanC commented Apr 1, 2024

@intel/llvm-gatekeepers Can you merge this please

@martygrant martygrant merged commit d6340b6 into intel:sycl Apr 1, 2024
13 checks passed
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