Skip to content

[UR] Check for memory alignment before calling clEnqueueMemFillINTEL_fn in OpenCL #18423

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

Merged
merged 3 commits into from
May 23, 2025

Conversation

martygrant
Copy link
Contributor

@martygrant martygrant commented May 12, 2025

Fixes oneapi-src/unified-runtime#2440

clEnqueueMemFillINTEL returns CL_INVALID_VALUE if dst_ptr is NULL, or if dst_ptr is not aligned to pattern_size bytes.

This PR adds a memory alignment check before calling clEnqueueMemFillINTEL to ensure we can safely call it, otherwise take the host side copy path.

Re-enables the USM/fill_any_size.cpp and also moves checkUSMImplAlignment from Hip into a common header so it can be used in both Hip and OpenCL.

@martygrant martygrant marked this pull request as ready for review May 14, 2025 12:57
@martygrant martygrant requested review from a team as code owners May 14, 2025 12:57
@martygrant
Copy link
Contributor Author

Arc fails tracked here #18416

@kbenzie
Copy link
Contributor

kbenzie commented May 14, 2025

Arc fails tracked here #18416

Those are not the same tests.

@martygrant
Copy link
Contributor Author

@kbenzie oops! I don't think there is an open issue for these ones yet that I can find. They failed on another PR today #18457. I'll open one.

Unrelated to the PR so should be fine.

@martygrant
Copy link
Contributor Author

Created an issue for them here #18463

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

E2E test change LGTM

martygrant added a commit that referenced this pull request May 21, 2025
…eueCommandForCG (#18517)

Add UR error code when throwing exception from
Scheduler::enqueueCommandForCG.
Remove message string from non adapter specific error exception in
checkUrResult as it's only used in the adapter specific error path.

This is related to
oneapi-src/unified-runtime#2440 and
#18423 where `clEnqueueMemFillINTEL`
was failing with -30 (`CL_INVALID_VALUE`) but no detail was provided in
the log:
```
terminate called after throwing an instance of 'sycl::_V1::exception'
  what():  Enqueue process failed.
Aborted (core dumped)
```
This is because the first exception thrown from `checkUrResult` was
being hidden by a second exception thrown from
`Scheduler::enqueueCommandForCG`. I've modified this second exception to
include the UR result. The output now looks like:
```
terminate called after throwing an instance of 'sycl::_V1::exception'
  what():  Enqueue process failed.
Native API failed. Native API returns: 4 (UR_RESULT_ERROR_INVALID_VALUE)
```
@martygrant martygrant force-pushed the martin/checkAlignmentUSMFill branch from eb282a1 to 8aab8c8 Compare May 21, 2025 14:15
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I suspect there probably needs to be some sort of alignment check in the Level Zero adapter also, but I'm not sure what the Level Zero alignment requirements are so this should be done in a separate PR.

@martygrant
Copy link
Contributor Author

martygrant commented May 21, 2025

GEN 12 fail tracked with #18577
BMG fails tracked with #18576
L0V2 CTS fail tracked with #18610

@martygrant
Copy link
Contributor Author

@bashbaug I've opened an issue to look at the L0 adapter

@martygrant martygrant force-pushed the martin/checkAlignmentUSMFill branch from edba358 to 5ef4a39 Compare May 21, 2025 15:55
@martygrant martygrant force-pushed the martin/checkAlignmentUSMFill branch from 5ef4a39 to 204a2ec Compare May 22, 2025 09:23
@martygrant martygrant force-pushed the martin/checkAlignmentUSMFill branch from 204a2ec to 2d7d393 Compare May 22, 2025 11:53
… OpenCL. Use checkUSMImplAlignment from the Hip adapter for this but moved it to a common header.
…ke single *pointer and deference at calls sites where double** was being passed.
@martygrant martygrant force-pushed the martin/checkAlignmentUSMFill branch from 2d7d393 to 4aa4f89 Compare May 22, 2025 15:13
@martygrant martygrant merged commit f009896 into intel:sycl May 23, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OpenCL][USM] urEnqueueUSMFill incorrectly assumes destination memory alignment and fails
6 participants