[SYCL][Graph] Implement dynamic local accessors by fabiomestre · Pull Request #18437 · intel/llvm

fabiomestre · 2025-05-13T10:05:10Z

Implements the dynamic_local_accessor class with compiler support.
Refactor the recently added dynamic_work_group_memory class to only use one impl member variable. This brings it closer to the design of other sycl classes and avoids future ABI break issues.
There are 2 ABI breaking changes. However, they are both related to the dynamic_work_group_memory class whose specification has not been merged yet and is not yet officially supported.

…l_accessor_impl

sycl/source/detail/graph_impl.hpp

fabiomestre · 2025-05-19T17:28:55Z

Missing reviews from @intel/llvm-reviewers-runtime and @intel/dpcpp-cfe-reviewers. Could someone from these groups have a look at this PR?

EwanC

Looks good to me, but I'm not as familiar with this code or Ben/Konrad, so should get one or both of them to approve this too.

clang/test/CodeGenSYCL/dynamic_local_accessor.cpp

sycl/include/sycl/handler.hpp

keyradical

LGTM!

aelovikov-intel · 2025-05-21T15:54:19Z

sycl/include/sycl/handler.hpp

-  setArgHelper(int ArgIndex,
-               ext::oneapi::experimental::detail::dynamic_work_group_memory_base
-                   &DynWorkGroupBase);
+  template <typename DataT, typename PropertyListT>


nit: would it make sense to only have declaration here and provide out-of-class definition somewhere in the graph headers?

I think that it makes sense to keep this in the handler header, at least for this PR. There is already a precedent to having setArgHelper() implementations in the handler for existing graph functionality (e.g. dynamic_parameter) and other extensions (e.g. work_group_memory). Moving only part of the implementation to the graph headers can be confusing. However, if you think that the handler header is getting too large / full of extension functionality, we can consider possible refactoring options as part of future work.

fabiomestre · 2025-05-23T09:50:35Z

Friendly ping to @intel/dpcpp-cfe-reviewers. Would appreciate a review on this PR.

fabiomestre · 2025-05-26T18:17:06Z

@intel/llvm-gatekeepers This PR should be ready to merge.

After intel#18437, the runtime library is producing a warning about an unused variable AccTarget in handler.cpp. This is due to the variable only being used in assert, which may in turn be removed when assertions are disabled. This commit removes the variable in favor of making the conversion inside the assert. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2025-05-27T06:19:47Z

Post-commit failure addressed in #18675.

After #18437, the runtime library is producing a warning about an unused variable AccTarget in handler.cpp. This is due to the variable only being used in assert, which may in turn be removed when assertions are disabled. This commit removes the variable in favor of making the conversion inside the assert. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

aelovikov-intel · 2025-06-04T16:02:09Z

sycl/test/abi/sycl_symbols_linux.dump

 _ZN4sycl3_V17handler11storeRawArgEPKvm
 _ZN4sycl3_V17handler12addReductionERKSt10shared_ptrIKvE
-_ZN4sycl3_V17handler12setArgHelperEiRNS0_3ext6oneapi12experimental6detail30dynamic_work_group_memory_baseE
+_ZN4sycl3_V13ext6oneapi12experimental6detail30dynamic_work_group_memory_base18updateWorkGroupMemEm


These aren't sorted, @fabiomestre how did you generate this diff? Was it the script or did you do it manually?

@fabiomestre , ping.

It has been a while so I don't remember exactly. I believe I used the script, for the initial PR but there were changes as a result of review comments, maybe I didn't use it at that point and assumed that the CI would tell me if anything was wrong.

I tried to run the script just now and it doesn't show any changes. How do I check whether this files are correctly sorted or not? If this is important, wouldn't it make sense for the CI to enforce it?

Please let me know if there is anything that still needs to be fixed at this point as a result of this PR. I can look into it and open a follow up PR.

Nevermind, I ran the script again and got the diff. Please review: https://github.com/intel/llvm/pull/18924/files

Note that it also showed diff for changes unrelated to this PR (e.g. submit_without_events).

I've been thinking that the CI should check if the lines are sorted, but I wasn't sure if breaks when people manually edit the file, or if difference in the development environment (e.g. python version) make the script itself produce different output, hence my questions.

If that's always some manual edit, then we can enforce sorting, otherwise some tooling change is required first.

fabiomestre changed the title ~~Implement dynamic local accessors~~ [SYCL][Graph] Implement dynamic local accessors May 13, 2025

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 10:05 — with GitHub Actions Failure

fabiomestre force-pushed the fabio/v2_dynamic_local_accessor_impl branch from 56eaa40 to 32945a2 Compare May 13, 2025 12:23

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 12:24 — with GitHub Actions Error

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 12:31 — with GitHub Actions Error

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 12:38 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 13:19 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 13:51 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 13:56 — with GitHub Actions Error

fabiomestre force-pushed the fabio/v2_dynamic_local_accessor_impl branch from 00f1bbf to da31888 Compare May 13, 2025 14:02

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 14:03 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 14:23 — with GitHub Actions Error

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 14:49 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 15:24 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 17:35 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 18:09 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 18:13 — with GitHub Actions Error

fabiomestre force-pushed the fabio/v2_dynamic_local_accessor_impl branch from db1d142 to 19e107c Compare May 13, 2025 18:16

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 18:16 — with GitHub Actions Failure

fabiomestre marked this pull request as ready for review May 13, 2025 18:43

fabiomestre requested review from a team as code owners May 13, 2025 18:43

fabiomestre requested review from Bensuo and aelovikov-intel May 13, 2025 18:43

fabiomestre had a problem deploying to WindowsCILock May 13, 2025 18:46 — with GitHub Actions Failure

fabiomestre temporarily deployed to WindowsCILock May 15, 2025 17:52 — with GitHub Actions Inactive

fabiomestre temporarily deployed to WindowsCILock May 15, 2025 18:30 — with GitHub Actions Inactive

Fabio Mestre added 2 commits May 19, 2025 13:33

Merge remote-tracking branch 'origin/sycl' into fabio/v2_dynamic_loca…

e6a172a

…l_accessor_impl

Address review comments

34249d5

fabiomestre had a problem deploying to WindowsCILock May 19, 2025 13:35 — with GitHub Actions Failure

fabiomestre had a problem deploying to WindowsCILock May 19, 2025 13:59 — with GitHub Actions Failure

Update windows symbols

773ef29

fabiomestre temporarily deployed to WindowsCILock May 19, 2025 16:50 — with GitHub Actions Inactive

keyradical reviewed May 19, 2025

View reviewed changes

sycl/source/detail/graph_impl.hpp Outdated Show resolved Hide resolved

fabiomestre temporarily deployed to WindowsCILock May 19, 2025 17:48 — with GitHub Actions Inactive

EwanC approved these changes May 21, 2025

View reviewed changes

clang/test/CodeGenSYCL/dynamic_local_accessor.cpp Outdated Show resolved Hide resolved

sycl/include/sycl/handler.hpp Outdated Show resolved Hide resolved

keyradical approved these changes May 21, 2025

View reviewed changes

aelovikov-intel approved these changes May 21, 2025

View reviewed changes

Fznamznon approved these changes May 26, 2025

View reviewed changes

Address review comments

703d3eb

fabiomestre temporarily deployed to WindowsCILock May 26, 2025 12:22 — with GitHub Actions Inactive

fabiomestre temporarily deployed to WindowsCILock May 26, 2025 12:51 — with GitHub Actions Inactive

steffenlarsen merged commit c20712f into intel:sycl May 27, 2025
24 checks passed

steffenlarsen mentioned this pull request May 27, 2025

[SYCL][NFC] Avoid unused variable AccTarget #18675

Merged

aelovikov-intel reviewed Jun 4, 2025

View reviewed changes

Conversation

fabiomestre commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fabiomestre commented May 19, 2025

Uh oh!

EwanC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keyradical left a comment

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel May 21, 2025

Choose a reason for hiding this comment

Uh oh!

fabiomestre May 26, 2025

Choose a reason for hiding this comment

Uh oh!

fabiomestre commented May 23, 2025

Uh oh!

fabiomestre commented May 26, 2025

Uh oh!

Uh oh!

steffenlarsen commented May 27, 2025

Uh oh!

aelovikov-intel Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

fabiomestre Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

fabiomestre Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fabiomestre commented May 13, 2025 •

edited

Loading

fabiomestre Jun 11, 2025 •

edited

Loading