Skip to content

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

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
Open
wants to merge 2 commits into
base: sycl
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

fabiomestre
Copy link
Contributor

@fabiomestre fabiomestre commented May 13, 2025 •
edited
Loading

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

@fabiomestre fabiomestre changed the title Implement dynamic local accessors [SYCL][Graph] Implement dynamic local accessors May 13, 2025
@fabiomestre fabiomestre force-pushed the fabio/v2_dynamic_local_accessor_impl branch from 56eaa40 to 32945a2 Compare May 13, 2025 12:23
@fabiomestre fabiomestre force-pushed the fabio/v2_dynamic_local_accessor_impl branch from 00f1bbf to da31888 Compare May 13, 2025 14:02
@fabiomestre fabiomestre force-pushed the fabio/v2_dynamic_local_accessor_impl branch from db1d142 to 19e107c Compare May 13, 2025 18:16
@fabiomestre fabiomestre marked this pull request as ready for review May 13, 2025 18:43
@fabiomestre fabiomestre requested review from a team as code owners May 13, 2025 18:43
@fabiomestre
Implement dynamic local accessors
cb78e9d 
* 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.
@fabiomestre fabiomestre force-pushed the fabio/v2_dynamic_local_accessor_impl branch from 19e107c to cb78e9d Compare May 13, 2025 19:08
EwanC
EwanC reviewed May 14, 2025
View reviewed changes
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Couple comments from a first pass

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

local_accessor<DataT, Dimensions> get() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

konradkusiak97
konradkusiak97 reviewed May 14, 2025
View reviewed changes
clang/lib/Sema/SemaSYCL.cpp
Comment on lines +4820 to +4828

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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,

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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why [[maybe_unused]] here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EwanC EwanC EwanC left review comments

@konradkusiak97 konradkusiak97 konradkusiak97 left review comments

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

@aelovikov-intel aelovikov-intel Awaiting requested review from aelovikov-intel aelovikov-intel is a code owner automatically assigned from intel/llvm-reviewers-runtime

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fabiomestre @EwanC @konradkusiak97