Skip to content

[SYCL][Graph] Make SYCL-Graph functions thread-safe #10778

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 9 commits into from
Aug 23, 2023

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Aug 10, 2023

This PR makes the new APIs defined by sycl_ext_oneapi_graph thread safe.

Authors

Co-authored-by: Pablo Reble pablo.reble@intel.com
Co-authored-by: Julian Miller julian.miller@intel.com
Co-authored-by: Ben Tracy ben.tracy@codeplay.com
Co-authored-by: Ewan Crawford ewan@codeplay.com
Co-authored-by: Maxime France-Pillois maxime.francepillois@codeplay.com

mfrancepillois and others added 3 commits August 10, 2023 15:04
* [SYCL][Graph] Makes command graph functions thread-safe

Addresses comments made on the first PR commit.
Mutexes are now added to Graph implementation entry points
instead of end points as was the case in the previous commit.
Adds "build_pthread_inc" lit test macro to facilitate the
compilation of the threading tests.
Removes std::barrier (std-20) dependency in threading tests.

Addresses Issue: #85

* [SYCL][Graph] Makes command graph functions thread-safe

Moves threading tests that do not require a device to run to unitests

* Update sycl/source/detail/graph_impl.cpp

Co-authored-by: Ben Tracy <ben.tracy@codeplay.com>

* [SYCL][Graph] Makes command graph functions thread-safe

Adds some comments.

* Update sycl/source/handler.cpp

Co-authored-by: Pablo Reble <pablo.reble@intel.com>

* Update sycl/source/detail/graph_impl.hpp

Co-authored-by: Ewan Crawford <ewan@codeplay.com>

* [SYCL][Graph] Makes command graph functions thread-safe

Adds dedidacted sub-class to unitests for multi-threading unitests

* [SYCL][Graph] Makes command graph functions thread-safe

Adds comments

* [SYCL][Graph] thread-safe: bug fix after rebase

---------

Co-authored-by: Ben Tracy <ben.tracy@codeplay.com>
Co-authored-by: Pablo Reble <pablo.reble@intel.com>
Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Removes the test-e2e dependency to graph_impl.hpp by changing the e2e test
to an unitests.
@EwanC EwanC requested review from a team as code owners August 10, 2023 15:19
@EwanC EwanC requested a review from cperkinsintel August 10, 2023 15:19
@EwanC EwanC marked this pull request as draft August 11, 2023 09:30
@EwanC EwanC marked this pull request as ready for review August 14, 2023 08:07
@EwanC EwanC requested a review from steffenlarsen August 14, 2023 08:07
@EwanC EwanC marked this pull request as draft August 14, 2023 09:45
@EwanC
Copy link
Contributor Author

EwanC commented Aug 14, 2023

Marked this as draft again, as we have discovered that graph enqueue isn't quite thread-safe yet

EwanC and others added 2 commits August 15, 2023 09:04
Fixes memory leak issues that occurred during multithreads graph finalization
@EwanC EwanC marked this pull request as ready for review August 15, 2023 10:37
@EwanC
Copy link
Contributor Author

EwanC commented Aug 18, 2023

Marked this as draft again, as we have discovered that graph enqueue isn't quite thread-safe yet

This issue has now been resolved, so PR is Open and ready for review from @intel/llvm-reviewers-runtime

EwanC and others added 3 commits August 18, 2023 16:23
Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
…this object (#302)

The way exec_graph_impl is used and the C++ semantic prevent concurrent access the destructor of this object.
@EwanC EwanC requested a review from steffenlarsen August 22, 2023 13:41
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!

@steffenlarsen steffenlarsen merged commit c8c64a6 into intel:sycl Aug 23, 2023
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