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] Fix recording in_order queue with empty nodes #246

Merged

Conversation

mfrancepillois
Copy link
Collaborator

Adding a empty node to a recorded in-order queue resulted in inconsistent dependencies between nodes.
This patch fixes this issues and simplifies the adding of empty nodes.
Unitests have been added to check node dependencies when recording an in_order queue with and without empty nodes.

Fixes Issue: #239

@mfrancepillois mfrancepillois added the Graph Implementation Related to DPC++ implementation and testing label Jun 30, 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.

Looks great, always a nice patch when removing code fixes an issue.

@mfrancepillois mfrancepillois force-pushed the maxime/in_order_queue_empty_node_fix branch from 0508e42 to 1b6dd83 Compare July 3, 2023 08:18
@Bensuo
Copy link
Collaborator

Bensuo commented Jul 4, 2023

If I'm not mistaken this change also changes the default behaviour of the handler when Graphs are not involved which I'm not sure about. Now an empty command group is passed to the scheduler instead of doing nothing and just returning an empty event.

The potential for unnecessary overhead there concerns me, and in general I think we should avoid modifying the default behaviour unless it is necessary or it improves it in some way.

@mfrancepillois mfrancepillois force-pushed the maxime/in_order_queue_empty_node_fix branch 3 times, most recently from a16914d to 78cbfc8 Compare July 4, 2023 11:04
@mfrancepillois
Copy link
Collaborator Author

If I'm not mistaken this change also changes the default behaviour of the handler when Graphs are not involved which I'm not sure about. Now an empty command group is passed to the scheduler instead of doing nothing and just returning an empty event.

The potential for unnecessary overhead there concerns me, and in general I think we should avoid modifying the default behaviour unless it is necessary or it improves it in some way.

Thanks for your review.
You are right, we should avoid sending empty node to scheduler in default mode.
I modified the code patch to keep the default behaviour as it was.

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, just some nitpicks!

sycl/source/handler.cpp Outdated Show resolved Hide resolved
sycl/unittests/Extensions/CommandGraph.cpp Outdated Show resolved Hide resolved
sycl/source/handler.cpp Outdated Show resolved Hide resolved
sycl/unittests/Extensions/CommandGraph.cpp Show resolved Hide resolved
sycl/unittests/Extensions/CommandGraph.cpp Show resolved Hide resolved
Adding a empty node to a recorded in-order queue resulted
in inconsistent dependencies between nodes.
This patch fixes this issues and simplifies the adding of empty nodes.
Unitests have been added to check node dependencies when
recording an in_order queue with and without empty nodes.

Fixes Issue: #239
@mfrancepillois mfrancepillois force-pushed the maxime/in_order_queue_empty_node_fix branch from 78cbfc8 to 7f683f8 Compare July 5, 2023 09:16
@mfrancepillois mfrancepillois merged commit 61a88ab into sycl-graph-develop Jul 5, 2023
@mfrancepillois mfrancepillois deleted the maxime/in_order_queue_empty_node_fix branch July 5, 2023 14:54
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