Skip to content

[UR][OpenCL] Add missing mapping for CL_INVALID_KERNEL to UR OpenCL adapter and handle CL_INVALID_KERNEL_DEFINITION in urKernelCreate #18182

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

Open
wants to merge 2 commits into
base: sycl
Choose a base branch
from

Conversation

martygrant
Copy link
Contributor

@martygrant martygrant commented Apr 24, 2025

Fixes oneapi-src/unified-runtime#2555

Also handles checking for CL_INVALID_KERNEL_DEFINITION inside urKernelCreate.

@martygrant
Copy link
Contributor Author

Failing test for Basic/sycl_2020_images/host_task_unsampled_image_write.cpp tracked here #18085

@martygrant martygrant marked this pull request as ready for review April 28, 2025 10:47
@martygrant martygrant requested a review from a team as a code owner April 28, 2025 10:47
@@ -46,6 +46,7 @@ ur_result_t mapCLErrorToUR(cl_int Result) {
case CL_INVALID_PROGRAM_EXECUTABLE:
return UR_RESULT_ERROR_INVALID_PROGRAM_EXECUTABLE;
case CL_INVALID_KERNEL_NAME:
case CL_INVALID_KERNEL_DEFINITION:
Copy link
Contributor

@kbenzie kbenzie Apr 28, 2025

Choose a reason for hiding this comment

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

CL_INVALID_KERNEL_DEFINITION if the function definition for __kernel function given by kernel_name such as the number of arguments, the argument types are not the same for all devices for which the program executable has been built.

I don't think this is the correct mapping. We don't have a direct mapping. Claiming this error condition is an invalid kernel name is misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah probably best to add a new enum for this, I'll add it in a new commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need that.

We could handle this only in the entry points that might return this, return UR_RESULT_ERROR_ADAPTER_SPECIFIC and set the message to "<entry point name> failing with CL_INVALID_KERNEL_DEFINITION".

I'm reasonably sure the other adapters won't have a use for a new enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated it to do this now instead

@martygrant martygrant force-pushed the martin/openclErrorMapping branch from 714a122 to 3cb06cb Compare April 28, 2025 15:19
@martygrant martygrant requested a review from a team as a code owner April 28, 2025 15:19
@martygrant martygrant force-pushed the martin/openclErrorMapping branch from 3cb06cb to 8e04857 Compare April 28, 2025 16:49
@martygrant martygrant changed the title [UR][OpenCL] Add missing mapping for CL_INVALID_KERNEL and CL_INVALID_KERNEL_DEFINITION to UR OpenCL adapter [UR][OpenCL] Add missing mapping for CL_INVALID_KERNEL to UR OpenCL adapter and handle CL_INVALID_KERNEL_DEFINITION in urKernelCreate Apr 28, 2025
if (CLResult == CL_INVALID_KERNEL_DEFINITION) {
cl_adapter::setErrorMessage(
"urKernelCreate failing with CL_INVALID_KERNEL_DEFINITION",
UR_RESULT_ERROR_ADAPTER_SPECIFIC);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is supposed to be the error code from clCreateKernel

Suggested change
UR_RESULT_ERROR_ADAPTER_SPECIFIC);
CLResult);

Copy link
Contributor Author

@martygrant martygrant Apr 30, 2025

Choose a reason for hiding this comment

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

I checked other uses of setErrorMessage and they all seem to pass UR_RESULT_ERROR_ADAPTER_SPECIFIC and the function expects a ur_result_t, just to double check that's correct?

Also looks like I removed the definition of setErrorMessage in the OpenCL adapter as there was no usage of it. I'll add it back in with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

ur_result_t urAdapterGetLastError(
    ur_adapter_handle_t hAdapter,
    const char **ppMessage,
    int32_t *pError);

It's supposed to match pError which has type int32_t.

Since you say all uses of setErrorMessage take a ur_result_t it seems like we need to change all of them. That can be done in another PR, create an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #18260 for this

martygrant and others added 2 commits April 30, 2025 13:47
…d check for CL_INVALID_KERNEL_DEFINITION inside urKernelCreate.
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.

[OpenCL] Missing mapping from CL_INVALID_KERNEL to UR_RESULT_ERROR_INVALID_KERNEL
2 participants