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] Add tests checking event status querying #244

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

mfrancepillois
Copy link
Collaborator

Checks the info::event_command_status on an event returned from graph submission.

Closes Issue: #95

@mfrancepillois mfrancepillois added the Graph Implementation Related to DPC++ implementation and testing label Jun 29, 2023
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM, one very minor comment

sycl/test-e2e/Graph/Explicit/event_status_querying.cpp Outdated Show resolved Hide resolved
Checks the info::event_command_status on an event returned from graph submission.

Closes Issue: #95
Copy link
Collaborator

@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

Copy link
Collaborator

@julianmi julianmi left a comment

Choose a reason for hiding this comment

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

I think these tests are an useful addition. Do we only want to verify the event info though? Couldn't we call a host kernel that waits a short time instead? This would simplify the tests and make the expected two states of the event reproducible.

@mfrancepillois
Copy link
Collaborator Author

I think these tests are an useful addition. Do we only want to verify the event info though? Couldn't we call a host kernel that waits a short time instead? This would simplify the tests and make the expected two states of the event reproducible.

This PR addresses Issue #95.
This issue requires users to be able to get status info on event return from graph submission.
We found that this behaviour is already supported by the default implementation.
We therefore only added tests that check this by displaying event status.

Using a host_task is indeed a good idea, but unfortunately this feature is not supported by the graph implementation at the moment.

@julianmi
Copy link
Collaborator

julianmi commented Jul 5, 2023

I think these tests are an useful addition. Do we only want to verify the event info though? Couldn't we call a host kernel that waits a short time instead? This would simplify the tests and make the expected two states of the event reproducible.

This PR addresses Issue #95. This issue requires users to be able to get status info on event return from graph submission. We found that this behaviour is already supported by the default implementation. We therefore only added tests that check this by displaying event status.

Using a host_task is indeed a good idea, but unfortunately this feature is not supported by the graph implementation at the moment.

I am okay with merging this as it is. We might want to add an issue though to make these tests reproducible and simplify them in the future. E.g., a long counting loop could do the trick as well.

@mfrancepillois mfrancepillois merged commit d624663 into sycl-graph-develop Jul 5, 2023
@mfrancepillois mfrancepillois deleted the maxime/event-query-state-info branch July 5, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graph Implementation Related to DPC++ implementation and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants