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] Make queue fill use native functions #12702

Merged
merged 12 commits into from
May 2, 2024

Conversation

konradkusiak97
Copy link
Contributor

@konradkusiak97 konradkusiak97 commented Feb 13, 2024

This PR changes the queue.fill() implementation to make use of the native functions for a specific backend. It also unifies that implementation with the one for memset, since it is just an 8-bit subset operation of fill.

In the CUDA case, both memset and fill are currently calling urEnqueueUSMFill which depending on the size of the filling pattern calls either cuMemsetD8Async, cuMemsetD16Async, cuMemsetD32Async or commonMemSetLargePattern. Before this patch memset was using the same thing, just beforehand setting patternSize always to 1 byte which resulted in calling cuMemsetD8Async. In other backends, the behaviour is analogous.

The fill method was just invoking a parallel_for to fill the memory with the pattern which was making this operation quite slow.

This PR depends on:

@konradkusiak97
Copy link
Contributor Author

Looks like there is a bug in ROCM prior to 6.0.0 which causes hipMemset2D to behave incorrectly on host-pinned memory. I'm working on a workaround for this.

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.

Graph related changes LGTM

@konradkusiak97
Copy link
Contributor Author

The dependent patches in the UR are all merged now so this PR is further ready for review. Friendly ping to @steffenlarsen and @intel/unified-runtime-reviewers

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

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

@konradkusiak97
Copy link
Contributor Author

@intel/llvm-gatekeepers this is ready to be merged

@sarnex sarnex merged commit 46e49ec into intel:sycl May 2, 2024
12 checks passed
@steffenlarsen
Copy link
Contributor

@konradkusiak97 - It looks like this may have caused https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/Basic/out_of_order_queue_status.cpp to fail on Gen12. Could you please have a look?

@konradkusiak97
Copy link
Contributor Author

@konradkusiak97 - It looks like this may have caused https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/Basic/out_of_order_queue_status.cpp to fail on Gen12. Could you please have a look?

I can't reproduce this locally, even with a debug build but I can see the post-commit failures, will investigate further.

@aelovikov-intel
Copy link
Contributor

@konradkusiak97 - It looks like this may have caused https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/Basic/out_of_order_queue_status.cpp to fail on Gen12. Could you please have a look?

I can't reproduce this locally, even with a debug build but I can see the post-commit failures, will investigate further.

Any updates on this? Should we just revert this PR?

@konradkusiak97
Copy link
Contributor Author

@konradkusiak97 - It looks like this may have caused https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/Basic/out_of_order_queue_status.cpp to fail on Gen12. Could you please have a look?

I can't reproduce this locally, even with a debug build but I can see the post-commit failures, will investigate further.

Any updates on this? Should we just revert this PR?

I'm still working on the fix so let's revert it for now.

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.

7 participants