-
Notifications
You must be signed in to change notification settings - Fork 734
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
Conversation
Looks like there is a bug in ROCM prior to 6.0.0 which causes |
There was a problem hiding this 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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UR LGTM
@intel/llvm-gatekeepers this is ready to be merged |
@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. |
This reverts commit 46e49ec.
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 eithercuMemsetD8Async
,cuMemsetD16Async
,cuMemsetD32Async
orcommonMemSetLargePattern
. Before this patch memset was using the same thing, just beforehand setting patternSize always to 1 byte which resulted in callingcuMemsetD8Async
. 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: