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

Add a function for stream to wait for a tag #746

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

deukhyun-cha
Copy link
Contributor

This is to implement a feature described in Add support a stream to synchronize for a specific event #512.

@deukhyun-cha
Copy link
Contributor Author

I would appreciate some help on testing especially for dpcpp and metal backends if possible.

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 74.94%. Comparing base (39908af) to head (c04baf6).

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #746      +/-   ##
===============================================
- Coverage        74.97%   74.94%   -0.03%     
===============================================
  Files              298      298              
  Lines            19487    19497      +10     
===============================================
+ Hits             14610    14613       +3     
- Misses            4877     4884       +7     
Files Coverage Δ
src/occa/internal/api/metal/commandQueue.hpp 0.00% <ø> (ø)
src/occa/internal/api/metal/polyfill.cpp 0.00% <0.00%> (ø)
src/occa/internal/modes/serial/stream.cpp 75.00% <0.00%> (-10.72%) ⬇️
src/c/base.cpp 84.00% <0.00%> (-1.72%) ⬇️
src/core/base.cpp 71.27% <0.00%> (-1.55%) ⬇️
src/core/stream.cpp 77.04% <0.00%> (-2.62%) ⬇️

... and 2 files with indirect coverage changes

@deukhyun-cha
Copy link
Contributor Author

I would appreciate some help on testing especially for dpcpp and metal backends if possible.

I was able to run the example added here for serial, cuda, dpcpp, and openmp backends.

@deukhyun-cha deukhyun-cha marked this pull request as ready for review March 19, 2024 22:31
@deukhyun-cha
Copy link
Contributor Author

I was going through this change and feeling the interface should be done through the device like other stream related functions. If so, should it also work with current stream on the device (instead of taking the stream as an argument)? I would appreciate advice.

@kris-rowe
Copy link
Member

I was going through this change and feeling the interface should be done through the device like other stream related functions. If so, should it also work with current stream on the device (instead of taking the stream as an argument)? I would appreciate advice.

Since it is complete, I would leave the existing implementation in the PR as is.

There is already a waitFor function in device that waits on a given streamTag. Since streamTags are associated with the stream which was active when they were created, the existing implementation of device::waitFor is problematic in the case where the active stream has been changed by the user. To avoid this ambiguity, device::waitFor should be reimplemented later using the function added in this PR to synchronize the current stream. A separate API for waiting on a specific streamTag could be added to that class afterwards, if needed.

@deukhyun-cha
Copy link
Contributor Author

I was going through this change and feeling the interface should be done through the device like other stream related functions. If so, should it also work with current stream on the device (instead of taking the stream as an argument)? I would appreciate advice.

Since it is complete, I would leave the existing implementation in the PR as is.

There is already a waitFor function in device that waits on a given streamTag. Since streamTags are associated with the stream which was active when they were created, the existing implementation of device::waitFor is problematic in the case where the active stream has been changed by the user. To avoid this ambiguity, device::waitFor should be reimplemented later using the function added in this PR to synchronize the current stream. A separate API for waiting on a specific streamTag could be added to that class afterwards, if needed.

Thanks @kris-rowe, appreciate your comment.

@deukhyun-cha
Copy link
Contributor Author

deukhyun-cha commented Apr 3, 2024

I was able to build this for Metal backend and run stream examples (after updating for some fixes) on a Mac. The branch is updated.

@deukhyun-cha
Copy link
Contributor Author

@kris-rowe just wanted to check if you have any general comments for me to consider on this PR before the detailed review?

@kris-rowe kris-rowe merged commit 4ea065d into libocca:development Apr 17, 2024
7 of 8 checks passed
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.

Add support a stream to synchronize for a specific event
2 participants