Skip to content

Comments

[SYCL][Graph] Add spec wording for graph-owned memory allocations#18001

Merged
uditagarwal97 merged 12 commits intointel:syclfrom
reble:ben/async-alloc-graphs-spec
Jul 4, 2025
Merged

[SYCL][Graph] Add spec wording for graph-owned memory allocations#18001
uditagarwal97 merged 12 commits intointel:syclfrom
reble:ben/async-alloc-graphs-spec

Conversation

@Bensuo
Copy link
Contributor

@Bensuo Bensuo commented Apr 14, 2025 •
edited
Loading

  • Using sycl_ext_codeplay_async_memory_alloc extension (PR currently in review)
  • Spec wording for graph support of the feature
  • Usage guide guidance for library authors
  • Usage guide examples of explicit and queue recording usage with and without mem pools

@Bensuo Bensuo mentioned this pull request Apr 14, 2025
Merged
@Bensuo
[SYCL][Graph] Add spec wording for graph-owned memory allocations
960c904 
- Using sycl_ext_oneapi_async_memory_alloc extension
- Spec wording for graph support of the feature
- Usage guide guidance for library authors
- Usage guide examples of explicit and queue recording usage with and without mem pools
@Bensuo Bensuo force-pushed the ben/async-alloc-graphs-spec branch from d204eb0 to 960c904 Compare April 14, 2025 14:58
guoyejun
guoyejun reviewed Apr 15, 2025
View reviewed changes
gmlueck
gmlueck reviewed Apr 15, 2025
View reviewed changes
@Bensuo
Addressing review comments
8dc4075 
- Improve whole-graph update conditions list.
- Remove unnecessary non-normative note about returned pointers.
- Fix duplicate exception sections in finalize() definition.
kbenzie pushed a commit that referenced this pull request Apr 29, 2025
@Bensuo
[SYCL][Graph] Initial implementation of graph-owned device allocations (
b742079 
#18002)

- Allocations managed via new graph_mem_pool
- Device allocations use virtual memory
- Intercept async_alloc calls when adding nodes to graph
- New tests for functionality
- Update CommandGraph design docs 

Draft spec: #18001
@reble reble mentioned this pull request May 2, 2025
Merged
15 tasks
This was referenced May 15, 2025
Closed
Closed
Closed
AerialMantis
AerialMantis reviewed Jun 6, 2025
View reviewed changes
Bensuo added 2 commits June 10, 2025 14:23
@Bensuo
Addressing review comments
3610085 
- Remove references to removed features from async alloc spec
- Improve links to async alloc spec
- Minor rewording of spec changes
- New usage guide example using in-order queue
@Bensuo
Merge remote-tracking branch 'origin/sycl' into ben/async-alloc-graph…
75b0dde 
…s-spec
AerialMantis
AerialMantis reviewed Jun 13, 2025
View reviewed changes
EwanC
EwanC reviewed Jun 16, 2025
View reviewed changes
Bensuo added 2 commits June 17, 2025 13:44
@Bensuo
Addressing review comments
1e9ae55 
- Fix broken links to base spec
- Make get_required_mem_size noexcept
- Minor wording improvements
- Add end_recording to example snippets
@Bensuo
Add missing error descriptions for prohibited ops on alloc/free nodes
421bfa7
@Bensuo Bensuo marked this pull request as ready for review June 17, 2025 15:39
@Bensuo Bensuo requested a review from a team as a code owner June 17, 2025 15:39
@Bensuo Bensuo requested a review from reble June 17, 2025 15:39
@Bensuo
Copy link
Contributor Author

Bensuo commented Jun 17, 2025

@gmlueck Not sure if you've done a detailed review of this already but it should be good to go now if you would like to take another look at it.

AerialMantis
AerialMantis reviewed Jun 20, 2025
View reviewed changes
Bensuo added 2 commits June 24, 2025 16:46
@Bensuo
Addressing review comments
2612d89 
- Change error code for host/shared allocation use
- Reword finalize to clarify multiple executable graphs
@Bensuo
Add open issue for allocations as USM pointers, remove old issues/design
00e4bab
AerialMantis
AerialMantis approved these changes Jun 26, 2025
View reviewed changes
Copy link

@AerialMantis AerialMantis left a comment

Choose a reason for hiding this comment

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

Looks good!

@Bensuo Bensuo mentioned this pull request Jul 1, 2025
Open
gmlueck
gmlueck reviewed Jul 1, 2025
View reviewed changes
@gmlueck
Copy link
Contributor

gmlueck commented Jul 2, 2025

I will be OOO for the rest of the week, so I approved. I think you understand my concern in the remaining comments, and I trust that you will address them.

@Bensuo
Address review comments
211589e 
- Clarify zero-initialization behavior
- Clarify error when recording subgraph with allocations
@Bensuo
Copy link
Contributor Author

Bensuo commented Jul 4, 2025

@intel/llvm-gatekeepers This should be good to merge now, thanks!

@uditagarwal97 uditagarwal97 merged commit badd00a into intel:sycl Jul 4, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlueck gmlueck gmlueck approved these changes

@reble reble Awaiting requested review from reble reble is a code owner automatically assigned from intel/sycl-graphs-reviewers

+3 more reviewers

@guoyejun guoyejun guoyejun left review comments

@EwanC EwanC EwanC approved these changes

@AerialMantis AerialMantis AerialMantis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Bensuo @gmlueck @EwanC @guoyejun @AerialMantis @uditagarwal97