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][COMPAT] Memset API updated to support 2-byte and 4-byte memsets #11340

Closed
wants to merge 3 commits into from

Conversation

Alcpz
Copy link
Contributor

@Alcpz Alcpz commented Sep 28, 2023

  • The memory interface has changed:
    • fill has been removed
    • memset now offers a templated version (which works as fill did)
    • memset_d8, memset_d16, and memset_d32 were added to allow memsetting 1, 2 and 4 byte-sized values.

@Alcpz Alcpz requested a review from a team as a code owner September 28, 2023 13:22
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 13:25 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 13:49 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock October 6, 2023 12:15 — with GitHub Actions Inactive
@joeatodd
Copy link
Contributor

joeatodd commented Oct 6, 2023

Working through this PR, I see that we have previously been somewhat inconsistent about pitched_data. In a few places the _x dimension is described as 'number of elements' but it definitely ought to be in bytes. I will work through that.

@Alcpz Alcpz temporarily deployed to WindowsCILock October 6, 2023 12:37 — with GitHub Actions Inactive
@joeatodd joeatodd temporarily deployed to WindowsCILock October 17, 2023 10:09 — with GitHub Actions Inactive
@joeatodd joeatodd temporarily deployed to WindowsCILock October 17, 2023 10:29 — with GitHub Actions Inactive
@JackAKirk
Copy link
Contributor

@konradkusiak97 is this a duplication of your fill work? Maybe there should just be a single interface for this functionality?

@konradkusiak97
Copy link
Contributor

@konradkusiak97 is this a duplication of your fill work? Maybe there should just be a single interface for this functionality?

After #12702 is merged, the fill command will use backend-specific calls like cuMemsetD16, D32 etc. so I imagine it would be quite similar.

@JackAKirk
Copy link
Contributor

@konradkusiak97 is this a duplication of your fill work? Maybe there should just be a single interface for this functionality?

After #12702 is merged, the fill command will use backend-specific calls like cuMemsetD16, D32 etc. so I imagine it would be quite similar.

Yeah that is what I was thinking. That would mean that sycl spec already supports this functionality in theory and you have now implemented it in practice. Therefore this compat PR doesn't seem to be a good idea? Why not just translate cuda code to the intended part of the sycl specification?

@Alcpz
Copy link
Contributor Author

Alcpz commented Apr 9, 2024

In this case, SYCLcompat wraps over the fill functionality @konradkusiak97 has been working on (in the specific case of the CUDA backend). While I agree that we should translate that way directly, this serves both as an example of how to translate, and is needed for applications that have been already been translated.

@Alcpz
Copy link
Contributor Author

Alcpz commented Apr 15, 2024

Closed. Replaced by #13409

@Alcpz Alcpz closed this Apr 15, 2024
sommerlukas pushed a commit that referenced this pull request Apr 16, 2024
…#13409)

This PR replaces #11340

This PR extends the memory header to include 2 byte and 4 byte memsets.
- memset remains unchanged.
- 2D / 3D memsets are templated and wrap `sycl::fill`. Functionality
remains unchanged as it is exposed through `detail::memset<unsigned
char>`, equivalent to what we had before.
- memset_d16 and memset_d32 calls are added wrapped around `sycl::fill`
using 2-byte and 4-byte datatypes

Added tests for memset_d16 and memset_d32.
@Alcpz Alcpz deleted the Alcpz/memory-memset-update branch June 27, 2024 15:46
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.

4 participants