[SYCL][Graph] Implement dynamic local accessors #18437

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.

* 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.

EwanC

Couple comments from a first pass

EwanC · 2025-05-14T08:00:31Z

sycl/test/abi/sycl_symbols_linux.dump

@@ -3520,7 +3519,14 @@ _ZN4sycl3_V17handler11saveCodeLocENS0_6detail13code_locationE
 _ZN4sycl3_V17handler11saveCodeLocENS0_6detail13code_locationEb
 _ZN4sycl3_V17handler11storeRawArgEPKvm
 _ZN4sycl3_V17handler12addReductionERKSt10shared_ptrIKvE
-_ZN4sycl3_V17handler12setArgHelperEiRNS0_3ext6oneapi12experimental6detail30dynamic_work_group_memory_baseE


There are breaking changed here from a symbols being removed, is it possible to avoid that with the preview macro to guard code that will be removed in the next ABI breaking window?

Yes, I can make that change. I was hoping this wouldn't be needed since support for dynamic_work_group_memory was added very recently and the specification hasn't merged yet.

ahh I missed that these were to dynamic_work_group_memory, I think your right that this shouldn't be an issue then, but can you highlight in the PR description why we don't think this ABI break is an issue.

EwanC · 2025-05-14T08:05:24Z

sycl/include/sycl/ext/oneapi/experimental/graph.hpp

+  {
+  }
+
+  local_accessor<DataT, Dimensions> get() const {


In the spec PR I don't think we say this method is const, so that is something to update in the spec

sycl/source/detail/graph_impl.hpp

konradkusiak97 · 2025-05-14T14:21:44Z

clang/lib/Sema/SemaSYCL.cpp

+
+      SYCLIntegrationHeader::kernel_param_kind_t ParamKind =
+          SYCLIntegrationHeader::kind_accessor;
+
+      if (SemaSYCL::isSyclType(FieldTy, SYCLTypeAttr::dynamic_local_accessor)) {
+        ParamKind = SYCLIntegrationHeader::kind_dynamic_accessor;
+      }
+
+      Header.addParamDesc(ParamKind, Info,


Nit, this could be made into ternary operator. Also same below.

Suggested change

SYCLIntegrationHeader::kernel_param_kind_t ParamKind =

SYCLIntegrationHeader::kind_accessor;

if (SemaSYCL::isSyclType(FieldTy, SYCLTypeAttr::dynamic_local_accessor)) {

ParamKind = SYCLIntegrationHeader::kind_dynamic_accessor;

}

Header.addParamDesc(ParamKind, Info,

SYCLIntegrationHeader::kernel_param_kind_t ParamKind =

SemaSYCL::isSyclType(FieldTy, SYCLTypeAttr::dynamic_local_accessor)

? SYCLIntegrationHeader::kind_dynamic_accessor

: SYCLIntegrationHeader::kind_accessor;

Header.addParamDesc(ParamKind, Info,

konradkusiak97 · 2025-05-14T15:03:28Z

sycl/include/sycl/ext/oneapi/experimental/graph.hpp

@@ -623,9 +629,15 @@ __SYCL_TYPE(dynamic_work_group_memory) dynamic_work_group_memory
  /// @param Graph The graph associated with this object.
  /// @param Num Number of elements in the unbounded array DataT.
  dynamic_work_group_memory(
-      experimental::command_graph<graph_state::modifiable> Graph, size_t Num)
+      [[maybe_unused]] experimental::command_graph<graph_state::modifiable>


If some of the jenkins jobs are still using gcc 7.5, then those [[maybe_unused]] in the constructor will cause a failure because of a gcc bug, see #17736

But I guess it will show up on the pre-commit CI too. Maybe the gcc version was bumped already.

konradkusiak97 · 2025-05-14T19:53:54Z

sycl/include/sycl/handler.hpp

@@ -1802,20 +1837,37 @@ class __SYCL_EXPORT handler {
  // set_arg for graph dynamic_parameters
  template <typename T>
  void set_arg(int argIndex,
-               ext::oneapi::experimental::dynamic_parameter<T> &dynamicParam) {
+               [[maybe_unused]] ext::oneapi::experimental::dynamic_parameter<T>


Why [[maybe_unused]] here?

Bensuo · 2025-05-15T11:19:55Z

sycl/include/sycl/handler.hpp

+          [[maybe_unused]] ext::oneapi::experimental::dynamic_work_group_memory<
+              DataT, PropertyListT> &DynWorkGroupMem) {
+
+#ifndef __SYCL_DEVICE_ONLY__


Why is this required for dynamic work group memory but not the existing work group mem set_arg() which looks to be doing basically the same thing?

Bensuo · 2025-05-15T11:30:22Z

sycl/source/detail/graph_impl.cpp

+#endif
+
+dynamic_work_group_memory_base::dynamic_work_group_memory_base(
+    [[maybe_unused]] experimental::command_graph<graph_state::modifiable> Graph,


Any reason for [[maybe_unused]] here vs just having the graph parameter be unnamed? Since it is never used at all.

Bensuo · 2025-05-15T11:52:50Z

sycl/source/handler.cpp

+      auto *DynParamImpl = static_cast<
+          ext::oneapi::experimental::detail::dynamic_parameter_impl *>(Ptr);
+
+      registerDynamicParameter(DynParamImpl, Index + IndexShift);
+
+      auto *DynLocalAccessorImpl = static_cast<
+          ext::oneapi::experimental::detail::dynamic_local_accessor_impl *>(
+          DynParamImpl);


Do we need separate casts here? Can't we just cast to dynamic_local_accessor_impl * and pass that to registerDynamicParameter()?

Bensuo · 2025-05-15T11:56:54Z

sycl/test-e2e/Graph/Update/dyn_local_accessor_multiple.cpp

+// Extra run to check for immediate-command-list in Level Zero
+// RUN: %if level_zero %{env SYCL_PI_LEVEL_ZERO_USE_IMMEDIATE_COMMANDLISTS=1 %{l0_leak_check} %{run} %t.out 2>&1 | FileCheck %s --implicit-check-not=LEAK %}
+
+// Tests using more than one dynamic_work_group_memory object in the same node.


Suggested change

// Tests using more than one dynamic_work_group_memory object in the same node.

// Tests using more than one dynamic_local_accessor object in the same node.

Bensuo · 2025-05-15T11:57:09Z

sycl/test-e2e/Graph/Update/dyn_local_accessor_basic.cpp

+// Extra run to check for immediate-command-list in Level Zero
+// RUN: %if level_zero %{env SYCL_PI_LEVEL_ZERO_USE_IMMEDIATE_COMMANDLISTS=1 %{l0_leak_check} %{run} %t.out 2>&1 | FileCheck %s --implicit-check-not=LEAK %}
+
+// Tests updating dynamic_work_group_memory with a new size.


Suggested change

// Tests updating dynamic_work_group_memory with a new size.

// Tests updating dynamic_local_accessor with a new size.

Bensuo · 2025-05-15T11:57:24Z

sycl/test-e2e/Graph/Update/dyn_cgf_with_dyn_local_accessor.cpp

+// Extra run to check for immediate-command-list in Level Zero
+// RUN: %if level_zero %{env SYCL_PI_LEVEL_ZERO_USE_IMMEDIATE_COMMANDLISTS=1 %{l0_leak_check} %{run} %t.out 2>&1 | FileCheck %s --implicit-check-not=LEAK %}
+
+// Tests using a dynamic command-group object with dynamic_work_group_memory.


Suggested change

// Tests using a dynamic command-group object with dynamic_work_group_memory.

// Tests using a dynamic command-group object with dynamic_local_accessor.

Bensuo · 2025-05-15T11:57:47Z

sycl/test-e2e/Graph/Update/dyn_local_accessor_multiple_nodes.cpp

+// Extra run to check for immediate-command-list in Level Zero
+// RUN: %if level_zero %{env SYCL_PI_LEVEL_ZERO_USE_IMMEDIATE_COMMANDLISTS=1 %{l0_leak_check} %{run} %t.out 2>&1 | FileCheck %s --implicit-check-not=LEAK %}
+
+// Tests using a dynamic_work_group_memory with multiple nodes.


Suggested change

// Tests using a dynamic_work_group_memory with multiple nodes.

// Tests using a dynamic_local_accessor with multiple nodes.

Bensuo · 2025-05-15T12:03:34Z

sycl/unittests/Extensions/CommandGraph/Update.cpp

+  // Check that registering a dynamic object with a node from a graph that was
+  // not passed to its constructor throws.


This comment doesn't make sense now since it is checking that it doesn't throw. The error for this was removed in #18199 .

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 force-pushed the fabio/v2_dynamic_local_accessor_impl branch from 19e107c to cb78e9d Compare May 13, 2025 19:08

fabiomestre temporarily deployed to WindowsCILock May 13, 2025 19:08 — with GitHub Actions Inactive

fabiomestre temporarily deployed to WindowsCILock May 13, 2025 19:46 — with GitHub Actions Inactive

EwanC reviewed May 14, 2025

View reviewed changes

Address review comment

d9fc8f7

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

fabiomestre temporarily deployed to WindowsCILock May 14, 2025 12:52 — with GitHub Actions Inactive

konradkusiak97 reviewed May 14, 2025

View reviewed changes

Bensuo reviewed May 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][Graph] Implement dynamic local accessors #18437

[SYCL][Graph] Implement dynamic local accessors #18437

fabiomestre commented May 13, 2025 •

edited

Loading

EwanC left a comment

EwanC May 14, 2025

fabiomestre May 14, 2025

EwanC May 14, 2025

EwanC May 14, 2025

konradkusiak97 May 14, 2025

konradkusiak97 May 14, 2025

konradkusiak97 May 14, 2025

Bensuo May 15, 2025

Bensuo May 15, 2025

Bensuo May 15, 2025

Bensuo May 15, 2025

Bensuo May 15, 2025

Bensuo May 15, 2025

Bensuo May 15, 2025

Bensuo May 15, 2025

	// Tests using more than one dynamic_work_group_memory object in the same node.
	// Tests using more than one dynamic_local_accessor object in the same node.

	// Tests updating dynamic_work_group_memory with a new size.
	// Tests updating dynamic_local_accessor with a new size.

	// Tests using a dynamic command-group object with dynamic_work_group_memory.
	// Tests using a dynamic command-group object with dynamic_local_accessor.

	// Tests using a dynamic_work_group_memory with multiple nodes.
	// Tests using a dynamic_local_accessor with multiple nodes.

		// Check that registering a dynamic object with a node from a graph that was
		// not passed to its constructor throws.

[SYCL][Graph] Implement dynamic local accessors #18437

Are you sure you want to change the base?

[SYCL][Graph] Implement dynamic local accessors #18437

Conversation

fabiomestre commented May 13, 2025 • edited Loading

EwanC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiomestre commented May 13, 2025 •

edited

Loading