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

[SYCL][Graph] Implement dynamic_work_group_memory for SYCL-Graphs #17314

Merged
merged 3 commits into from
Mar 28, 2025

Conversation

konradkusiak97
Copy link
Contributor

@konradkusiak97 konradkusiak97 commented Mar 5, 2025

Implements dynamic_work_group_memory for SYCL-Graphs: #16712

With this PR we're able to update work_group_memory size on subsequent graph executions. We're also now able to use dynamic_work_group_memory with both lambdas and free function kernels.

@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from 7fb552a to a5e4bbf Compare March 5, 2025 13:13
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from a5e4bbf to 2a0c7c9 Compare March 5, 2025 13:15
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from 2a0c7c9 to f15fae7 Compare March 5, 2025 13:28
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from f15fae7 to 521b53e Compare March 5, 2025 13:40
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from 521b53e to 6813258 Compare March 5, 2025 13:45
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from 3948bfe to d2dd3f4 Compare March 5, 2025 16:00
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from a3140d1 to fc2c09a Compare March 5, 2025 20:35
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from fc2c09a to 3d79d2e Compare March 6, 2025 12:05
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from aee7b34 to 5352503 Compare March 6, 2025 13:22
@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from 5352503 to f5a2473 Compare March 6, 2025 17:29
@EwanC
Copy link
Contributor

EwanC commented Mar 21, 2025

pinging @intel/dpcpp-cfe-reviewers for review

Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

Are more kinds of work_group_memory coming?
The copy-paste in clang's code is somewhat unfortunate. Any chance the runtime library may not need to know which kind of work_group_memory is that?

@konradkusiak97
Copy link
Contributor Author

konradkusiak97 commented Mar 24, 2025

Are more kinds of work_group_memory coming? The copy-paste in clang's code is somewhat unfortunate. Any chance the runtime library may not need to know which kind of work_group_memory is that?

Short answer is yes. We're extending sycl-graph specification with dynamic_work_group_memory, dynamic_local_accessor and dynamic_accessor. These are outlined in the spec changes PR: #16712. Similarly to this PR, the plan is for all these new types to follow the similar pattern which wraps around the existing classes to add the update functionality in graphs. Altough actually they would be related to local_accessor and accessor and not work_group_memory. Specifically for work_group_memory, no, there are no more new kinds of it coming.

That's true, there has been a bit of a copy-paste in clang's code but AFAIK, this is required if we want to pass dynamic_work_group_memory type to a kernel. I can't think of a way in which the frontend doesn't need to know the exact kind of work_group_memory. I'll follow up with a refactor PR if I manage to find a solution.

@konradkusiak97
Copy link
Contributor Author

konradkusiak97 commented Mar 27, 2025

@intel/llvm-gatekeepers this is ready to be merged, thank you.

@EwanC
Copy link
Contributor

EwanC commented Mar 27, 2025

@intel/llvm-gatekeepers this is ready to be merged, thank you.

I think this still needs a @intel/llvm-reviewers-runtime approval

@konradkusiak97
Copy link
Contributor Author

@intel/llvm-gatekeepers this is ready to be merged, thank you.

I think this still needs a @intel/llvm-reviewers-runtime approval

That's true, gatekeepers, sorry for the oversight.

@konradkusiak97
Copy link
Contributor Author

@KseniyaTikhomirova could I get a review on this, please?

Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

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

sycl part LGTM

@konradkusiak97
Copy link
Contributor Author

@intel/llvm-gatekeepers this is now ready to merge

@konradkusiak97 konradkusiak97 force-pushed the dynamicWorkGroupMemWithFFKV2 branch from 6903f46 to 16f83ba Compare March 28, 2025 11:54
@martygrant martygrant merged commit 2abe0d6 into intel:sycl Mar 28, 2025
33 of 34 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.

8 participants