Skip to content

[SYCL] Allow batching for wait with a barrier commands #5307

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 1 commit into from
Jan 14, 2022

Conversation

smaslov-intel
Copy link
Contributor

Signed-off-by: Sergey V Maslov [email protected]

@smaslov-intel smaslov-intel requested a review from a team as a code owner January 13, 2022 21:57
@smaslov-intel
Copy link
Contributor Author

/summary:run

@@ -5379,6 +5379,22 @@ pi_result piEnqueueEventsWaitWithBarrier(pi_queue Queue,
PI_ASSERT(Queue, PI_INVALID_QUEUE);
PI_ASSERT(Event, PI_INVALID_EVENT);

// Submit dependent open command lists for execution, if any
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please clarify in the comment why we need to execute all command lists.
It doesn't look obvious why it is required.

Also this maybe not closely related to this PR but how we deal with the situation when events in the waitlist are associated with device(s) which are different from device where command list is created. It looks like it is illegal per L0 spec:
https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=zecommandlistappendbarrier#zecommandlistappendbarrier

The application must ensure the events are accessible by the device on which the command list was created.

In opencl requirement is less strict and it is enough for queue and events to be in the same context:
https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/clEnqueueBarrierWithWaitList.html

The context associated with events in event_wait_list and command_queue must be the same.

It doesn't look like we have a guarantee that all events in the list are associated with the same device or its subdevices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why we need to execute all command lists.

This is to ensure forward progress, and no deadlock.
If we don't submit commands being waited, then the wait may be infinite.

deal with the situation when events in the waitlist are associated with device(s) which are different from device where command list is created

That's the same as for all other commands, e.g. see zeCommandListAppendLaunchKernel:
https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=zecommandlistappend#_CPPv431zeCommandListAppendLaunchKernel24ze_command_list_handle_t18ze_kernel_handle_tPK16ze_group_count_t17ze_event_handle_t8uint32_tP17ze_event_handle_t

If this is an issue then we'd address it separately, and I don't know how at this moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to ensure forward progress, and no deadlock.
If we don't submit commands being waited, then the wait may be infinite.

Indeed, got it, thank you.

That's the same as for all other commands, e.g. see zeCommandListAppendLaunchKernel:

You are right, I didn't notice this until now.

@@ -5379,6 +5379,22 @@ pi_result piEnqueueEventsWaitWithBarrier(pi_queue Queue,
PI_ASSERT(Queue, PI_INVALID_QUEUE);
PI_ASSERT(Event, PI_INVALID_EVENT);

// Submit dependent open command lists for execution, if any
Copy link
Contributor

Choose a reason for hiding this comment

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

This is to ensure forward progress, and no deadlock.
If we don't submit commands being waited, then the wait may be infinite.

Indeed, got it, thank you.

That's the same as for all other commands, e.g. see zeCommandListAppendLaunchKernel:

You are right, I didn't notice this until now.

@bader bader changed the title [SYCL] allow batching for wait with a barrier commands [SYCL] Allow batching for wait with a barrier commands Jan 14, 2022
@bader bader merged commit bd1ed6a into intel:sycl Jan 14, 2022
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.

3 participants