[SYCL][Graph] Error on invalid buffer behaviour #287
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EwanC
reviewed
Aug 3, 2023
EwanC
reviewed
Aug 3, 2023
mfrancepillois
approved these changes
Aug 3, 2023
There was a problem hiding this comment.
LGTM.
@gmlueck mentioned in this comment you have tagged as reference to this PR: "Passing the property to begin_recording() is an assertion by the user that buffers may be safely recorded until the next call to end_recording() for this queue. "
Is there a reason not to implement this behaviour?
sycl/test-e2e/Graph/Inputs/assume_data_outlives_buffer_property.cpp
Outdated
Show resolved
Hide resolved
This is also a problem that would affect the explicit API, if you add a node which uses buffers then delete the host data you have the same problem. So it seemed to me this would be better placed as a graph property. Edit: Ah I see now that he has mentioned adding both. To me this seems unnecessary and a bit error prone, so I would be in favour of keeping on graph construction only. |
Bensuo
force-pushed
the
ben/buffer-invalid-behaviour
branch
from
f85028d to
993c0f3
Compare
reble
reviewed
Aug 3, 2023
EwanC
approved these changes
Aug 7, 2023
Removes the test-e2e dependency to graph_impl.hpp by changing the e2e test to an unitests.
- Error for buffers with write back enabled. - Error for host accessors to buffers in use by a graph. - Unit tests added for these cases - Fixed E2E tests which were not compliant with these restrictions.
- Add assume_data_outlives_buffer property - Add check to disallow buffers with host data without this property - Add a generic test for this. - Update E2E tests which now require the property
- Use weak_ptrs in both classes to prevent circular references and unnecessary lifetime extensions - Update unit tests for new properties
Bensuo
force-pushed
the
ben/buffer-invalid-behaviour
branch
from
26e1db5 to
d4e2858
Compare
mfrancepillois
force-pushed
the
sycl-graphs-improvements-fixes
branch
from
6af80ab to
e578f69
Compare
reble
approved these changes
Aug 8, 2023
EwanC
added a commit
that referenced
this pull request
Aug 9, 2023
* [SYCL][Graph] Makes command graph functions thread-safe (bugfix) Removes the test-e2e dependency to graph_impl.hpp by changing the e2e test to an unitests. * [SYCL][Graph] Error on invalid buffer behaviour - Error for buffers with write back enabled. - Error for host accessors to buffers in use by a graph. - Unit tests added for these cases - Fixed E2E tests which were not compliant with these restrictions. * [SYCL][Graph] Implement assume_data_outlives_buffer property - Add assume_data_outlives_buffer property - Add check to disallow buffers with host data without this property - Add a generic test for this. - Update E2E tests which now require the property * Update symbol dumps * [SYCL][Graph] Implement assume_buffer_outlives_graph property and checks * [SYCL][Graph] Add E2E test for assume_buffer_outlives_graph property * [SYCL][Graph] Fix circular reference between queue and graph_impl - Use weak_ptrs in both classes to prevent circular references and unnecessary lifetime extensions - Update unit tests for new properties * [SYCL][Graph] Remove unnecessary test line * [SYCL][Graph] Make graph use count in memobjects atomic * [SYCL][Graph] Make markNoLongerBeingUsedInGraph() thread safe --------- Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com> Co-authored-by: Ewan Crawford <ewan@codeplay.com>
EwanC
added a commit
that referenced
this pull request
Aug 10, 2023
* [SYCL][Graph] Makes command graph functions thread-safe (bugfix) Removes the test-e2e dependency to graph_impl.hpp by changing the e2e test to an unitests. * [SYCL][Graph] Error on invalid buffer behaviour - Error for buffers with write back enabled. - Error for host accessors to buffers in use by a graph. - Unit tests added for these cases - Fixed E2E tests which were not compliant with these restrictions. * [SYCL][Graph] Implement assume_data_outlives_buffer property - Add assume_data_outlives_buffer property - Add check to disallow buffers with host data without this property - Add a generic test for this. - Update E2E tests which now require the property * Update symbol dumps * [SYCL][Graph] Implement assume_buffer_outlives_graph property and checks * [SYCL][Graph] Add E2E test for assume_buffer_outlives_graph property * [SYCL][Graph] Fix circular reference between queue and graph_impl - Use weak_ptrs in both classes to prevent circular references and unnecessary lifetime extensions - Update unit tests for new properties * [SYCL][Graph] Remove unnecessary test line * [SYCL][Graph] Make graph use count in memobjects atomic * [SYCL][Graph] Make markNoLongerBeingUsedInGraph() thread safe --------- Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com> Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
property::graph::assume_data_outlives_bufferand associated check as described in this comment on the spec: [SYCL][Graph] Promote graph spec status to experimental intel/llvm#10473 (comment)