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] wait_and_throw invokes asynchronous handlers linked to the events that the event depends on. #12202

Open
wants to merge 5 commits into
base: sycl
Choose a base branch
from

Conversation

mmoadeli
Copy link
Contributor

@mmoadeli mmoadeli commented Dec 18, 2023

Modifies wait_and_throw to invoke asynchronous handlers linked to the events that an event depends on.
There is a corresponding PR to clarify specification.

@mmoadeli mmoadeli requested a review from a team as a code owner December 18, 2023 22:00
@mmoadeli mmoadeli changed the title [SYCL] Invoke asynchronous handlers linked to the events that the occurrence of an event relies on. [SYCL] Modifies wait_and_throw to invoke asynchronous handlers linked to the events that the occurrence of an event relies on. Dec 18, 2023
@mmoadeli mmoadeli changed the title [SYCL] Modifies wait_and_throw to invoke asynchronous handlers linked to the events that the occurrence of an event relies on. [SYCL] Modifies wait_and_throw to invoke asynchronous handlers linked to the events that an event depends on. Dec 18, 2023
@mmoadeli mmoadeli changed the title [SYCL] Modifies wait_and_throw to invoke asynchronous handlers linked to the events that an event depends on. [SYCL] wait_and_throw invokes asynchronous handlers linked to the events that an event depends on. Dec 18, 2023
@mmoadeli mmoadeli marked this pull request as draft December 18, 2023 23:02
@mmoadeli mmoadeli marked this pull request as ready for review December 19, 2023 00:02
@mmoadeli mmoadeli changed the title [SYCL] wait_and_throw invokes asynchronous handlers linked to the events that an event depends on. [SYCL] wait_and_throw invokes asynchronous handlers linked to the events that the event depends on. Dec 19, 2023
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Jan 8, 2024

@dm-vodopyanov a friendly request for review please.

@mmoadeli
Copy link
Contributor Author

mmoadeli commented Jan 18, 2024

@dm-vodopyanov, @Naghasan a friendly request for review please.

@AlexeySachkov
Copy link
Contributor

@mmoadeli, the PR doesn't require any extra approvals, since @steffenlarsen is a member of runtime-reviewers team. However, I'm a bit hesitant to merge it right now considering that the spec PR is still open (even though it is approved). Tagging @gmlueck as one of the reviewers for extra inputs.

Besides that, I wonder if this should be considered as a breaking change and put under -fpreview-breaking-changes mode? Note: I haven't looked into this or the spec PRs, just asking blindly to double-check

@mmoadeli
Copy link
Contributor Author

@mmoadeli, the PR doesn't require any extra approvals, since @steffenlarsen is a member of runtime-reviewers team. However, I'm a bit hesitant to merge it right now considering that the spec PR is still open (even though it is approved). Tagging @gmlueck as one of the reviewers for extra inputs.

Besides that, I wonder if this should be considered as a breaking change and put under -fpreview-breaking-changes mode? Note: I haven't looked into this or the spec PRs, just asking blindly to double-check

thanks @AlexeySachkov

@steffenlarsen
Copy link
Contributor

Since Greg is OOO, I am tagging @Pennycook and @AerialMantis on this. It seems to me like the spec clarification has 2 approvals, so I figure it will get in soon. As to whether this is a breaking change is a good question though.

Copy link
Contributor

@AerialMantis AerialMantis left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a suggestion for a further test to catch a further case highlighted in the recent spec discussions.


int main() {
{
test_exception_handler teh1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be worth also having a test for the case that @gmlueck highlighted in the spec PR where there is no dependency chain between the two kernels but they are enqueued to the same queue.

It was decided that in this case we should all errors caught by the queue, so if there is a call to wait on the queue then both exceptions should be thrown.

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 Sep 26, 2024
@mmoadeli
Copy link
Contributor Author

mmoadeli commented Oct 2, 2024

@AerialMantis It seems the corresponding spec PR is merged. Could this be merged in?

@github-actions github-actions bot removed the Stale label Oct 3, 2024
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.

4 participants