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.

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

[SYCL][Graph] Add support for fill and memset nodes in graphs #11472

Merged
merged 30 commits into from
Jan 11, 2024
Merged

[SYCL][Graph] Add support for fill and memset nodes in graphs #11472

merged 30 commits into from
Jan 11, 2024

Conversation

Bensuo
Copy link
Contributor

@Bensuo Bensuo commented Oct 9, 2023

  • Adds support for fill and memset nodes in graphs.
  • Supported on Level Zero only for now.
  • Adds E2E and unit tests for these new node types.
  • Minor modifications due to renaming of some UR functions.

@Bensuo Bensuo mentioned this pull request Oct 9, 2023
Merged
@Bensuo Bensuo temporarily deployed to WindowsCILock October 9, 2023 15:41 — with GitHub Actions Inactive
@Bensuo Bensuo temporarily deployed to WindowsCILock October 9, 2023 16:08 — with GitHub Actions Inactive
@al42and
Copy link
Contributor

al42and commented Oct 26, 2023

May I ask when this change and the associated UR PR are expected to be merged?

@Bensuo
Copy link
Contributor Author

Bensuo commented Oct 26, 2023

May I ask when this change and the associated UR PR are expected to be merged?

As far as I'm aware there isn't a clear timeline for merging the associated UR PR right now unfortunately, due to the adapter move still being in progress. Since these changes involve updates to the UR specification for command buffers it may be delayed until all adapters have been moved and the adapters branch is retired in favour using of the main branch again.

@al42and
Copy link
Contributor

al42and commented Oct 27, 2023

BTW, is it the expected behavior that

q.submit((sycl::handler & cgh) { cgh.memset(ptr_, 0, size_); });

does not work without this PR, but

q.memset(ptr_, 0, size_);

works fine in a graph on LevelZero devices with the current sycl branch (at least in a very simple test)?

EwanC
EwanC reviewed Dec 5, 2023
View reviewed changes
sycl/test-e2e/Graph/Explicit/usm_memset.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Graph/RecordReplay/usm_memset.cpp Outdated Show resolved Hide resolved
sycl/doc/design/CommandGraph.md Show resolved Hide resolved
EwanC
EwanC reviewed Dec 5, 2023
View reviewed changes
sycl/plugins/opencl/pi_opencl.cpp Outdated Show resolved Hide resolved
sycl/doc/design/CommandGraph.md Outdated Show resolved Hide resolved
EwanC
EwanC approved these changes Dec 7, 2023
View reviewed changes
sycl/test-e2e/Graph/Explicit/buffer_fill.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Graph/Explicit/usm_memset.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Graph/RecordReplay/buffer_fill.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Graph/RecordReplay/usm_memset.cpp Outdated Show resolved Hide resolved
kbenzie
kbenzie approved these changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

UR LGTM

@EwanC EwanC temporarily deployed to WindowsCILock January 9, 2024 10:54 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock January 9, 2024 11:25 — with GitHub Actions Inactive
@EwanC EwanC mentioned this pull request Jan 9, 2024
Merged
@EwanC EwanC temporarily deployed to WindowsCILock January 9, 2024 12:19 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock January 9, 2024 12:50 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock January 9, 2024 14:46 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock January 9, 2024 15:20 — with GitHub Actions Inactive
@EwanC
Copy link
Contributor

EwanC commented Jan 10, 2024

Thanks for the review @aelovikov-intel, I think I've addressed all your comments if you could take another look.

@EwanC
Copy link
Contributor

EwanC commented Jan 10, 2024

ping @intel/llvm-reviewers-cuda for review

jchlanda
jchlanda approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@jchlanda jchlanda left a comment

Choose a reason for hiding this comment

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

CUDA/HIP 👍

@EwanC
Copy link
Contributor

EwanC commented Jan 11, 2024

@intel/llvm-gatekeepers Can you merge this please

@steffenlarsen steffenlarsen merged commit 8ea0229 into intel:sycl Jan 11, 2024
12 checks passed
@EwanC
Copy link
Contributor

EwanC commented Jan 11, 2024 •
edited
Loading

This PR broke post-commit CI due to an unused variable, I'll look into it - #12362

@EwanC EwanC mentioned this pull request Jan 11, 2024
Merged
EwanC added a commit to reble/llvm that referenced this pull request Jan 11, 2024
@EwanC
[SYCL][Graph] Fix post-commit CI after PR 11472
cd611f1 
Ignore unused `Size` parameter from
`memory_manager::ext_oneapi_fill_cmd_buffer`.

This was introduced in PR intel#11472
and passed as `Req->MMemoryRange` from the command-group requirement
but not used for calling the underlying UR function, leading to a
`-Wunused-parameter` error.

It may be that this is needed to support buffers of dimension greater
than 1, which there are currently no graphs tests for. This will
be investigated as a follow-up once post-commit CI is unblocked
with this PR.
aelovikov-intel pushed a commit that referenced this pull request Jan 11, 2024
@EwanC
[SYCL][Graph] Fix post-commit CI after PR #11472 (#12362)
87de16b 
Ignore unused `Size` parameter from
`memory_manager::ext_oneapi_fill_cmd_buffer`.

This was introduced in PR #11472 and
passed as `Req->MMemoryRange` from the command-group requirement but not
used for calling the underlying UR function, leading to a
`-Wunused-parameter` error.

It may be that this is needed to support buffers of dimension greater
than 1, which there are currently no graphs tests for. This will be
investigated as a follow-up once post-commit CI is unblocked with this
PR.
@EwanC EwanC mentioned this pull request Jan 16, 2024
Merged
EwanC added a commit to reble/llvm that referenced this pull request Feb 6, 2024
@EwanC
[SYCL][Graph] Remove expired limitations
8bc511d 
The merged PR intel#11472
added support for `handler::fill` and `handler::memset`
nodes in a graph, while merged PR
intel#11474 added support
for `handler::prefetch` and `handler::mem_advise`.
@EwanC EwanC mentioned this pull request Feb 6, 2024
Merged
AlexeySachkov pushed a commit that referenced this pull request Feb 7, 2024
@EwanC
[SYCL][Graph] Remove expired limitations from specification (#12618)
87b55fd 
The merged PR #11472 added support for
`handler::fill` and `handler::memset` nodes in a graph, while merged PR
#11474 added support for
`handler::prefetch` and `handler::mem_advise`. Remove mentions of these
as limitations from the specification, as they are now supported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EwanC EwanC EwanC approved these changes

@mfrancepillois mfrancepillois mfrancepillois approved these changes

@aelovikov-intel aelovikov-intel aelovikov-intel approved these changes

@kbenzie kbenzie kbenzie approved these changes

@jchlanda jchlanda jchlanda approved these changes

@npmiller npmiller Awaiting requested review from npmiller npmiller was automatically assigned from intel/llvm-reviewers-cuda

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

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

Successfully merging this pull request may close these issues.
8 participants
@Bensuo @al42and @EwanC @aelovikov-intel @jchlanda @kbenzie @mfrancepillois @steffenlarsen