Skip to content

[SYCL][Graph] Add property for allowing host data created buffers #289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 9, 2023
Merged

[SYCL][Graph] Add property for allowing host data created buffers #289

merged 6 commits into from
Aug 9, 2023

Conversation

Bensuo
Copy link
Collaborator

@Bensuo Bensuo commented Aug 3, 2023 •
edited
Loading

  • Add wording which restricts buffers created with host data in graphs.
  • Add a property which disables those checks.
  • Add wording which clarifies that we do not currently extend buffer lifetimes and restrict their use.
  • Add a property which disables restrictions and represents a promise from the user that buffer lifetimes will exceed the graph lifetime.

Addresses feedback from this comment: intel#10473 (comment) and implemented in #287

@Bensuo
[SYCL][Graph] Add property for allowing host data created buffers
d639dcc 
- Add wording which restricts buffers created with host data in graphs
- Add a property which disables those checks
@Bensuo Bensuo added the Graph Specification Extension Specification related label Aug 3, 2023
@Bensuo Bensuo marked this pull request as draft August 3, 2023 14:25
@Bensuo
[SYCL][Graph] Add property for assuming buffer lfietimes outlive graph
caf0289 
- Add new property for allowing buffers with a lifetime promise
- Rework buffer limitations wording for this and general improvement
@Bensuo Bensuo marked this pull request as ready for review August 3, 2023 15:48
EwanC
EwanC reviewed Aug 4, 2023
View reviewed changes
@Bensuo
[SYCL][Graph] Addressing PR feedback
cf9072c 
- Add missing API modifications for properties
- Add punctuation
- Fix new property links
@Bensuo Bensuo force-pushed the ben/assume-data-outlives-buffer branch from 1c44d44 to cf9072c Compare August 4, 2023 16:32
reble
reble reviewed Aug 7, 2023
View reviewed changes
reble
reble reviewed Aug 7, 2023
View reviewed changes
reble
reble reviewed Aug 7, 2023
View reviewed changes
reble
reble approved these changes Aug 7, 2023
View reviewed changes
Copy link
Owner

@reble reble left a comment

Choose a reason for hiding this comment

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

LGTM

mfrancepillois
mfrancepillois approved these changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@mfrancepillois mfrancepillois left a comment

Choose a reason for hiding this comment

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

LGTM.
BTW good job/improvement. It makes the buffer lifetime limitations much clearer and easier to use than before.

EwanC and others added 2 commits August 9, 2023 10:07
@EwanC @reble
Apply suggestions from code review
3df32f9 
Co-authored-by: Pablo Reble <pablo.reble@intel.com>
@EwanC
Link to "Buffer Limitations" section
f1ef521
@EwanC EwanC merged commit b6c5c42 into pablo/promote-graph-spec Aug 9, 2023
@EwanC EwanC deleted the ben/assume-data-outlives-buffer branch August 9, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EwanC EwanC EwanC approved these changes

@reble reble reble approved these changes

@mfrancepillois mfrancepillois mfrancepillois approved these changes

@julianmi julianmi Awaiting requested review from julianmi

Assignees
No one assigned
Labels
Graph Specification Extension Specification related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Bensuo @EwanC @reble @mfrancepillois