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

Should binary_status[i] be set in clCreateProgramWithBinary if lengths[i] is zero or binaries[i] is NULL? #1203

Open
LingMan opened this issue Jul 8, 2024 · 3 comments
Labels
needs-cts-coverage The CTS needs to be extended

Comments

@LingMan
Copy link

LingMan commented Jul 8, 2024

Hi

Suppose that (lengths[i] == 0 || binaries[i] == NULL) && binary_status != NULL in clCreateProgramWithBinary.
Should binary_status[i] be set to CL_INVALID_VALUE in that case?

The spec currently says:

binary_status returns whether the program binary for each device specified in device_list was loaded successfully or not. It is an array of num_devices entries and returns CL_SUCCESS in binary_status[i] if binary was successfully loaded for device specified by device_list[i]; otherwise returns CL_INVALID_VALUE if lengths[i] is zero or if binaries[i] is a NULL value or CL_INVALID_BINARY in binary_status[i] if program binary is not a valid binary for the specified device. If binary_status is NULL, it is ignored.

CL_INVALID_VALUE if lengths or binaries is NULL or if any entry in lengths[i] is zero or binaries[i] is NULL.

CL_INVALID_BINARY if an invalid program binary was encountered for any device. binary_status will return specific status for each device.

If yes, it would be good to add the sentence "binary_status will return specific status for each device." to the documentation of the CL_INVALID_VALUE return value and to add "in binary_status[i]" to CL_INVALID_VALUE within the documentation of binary_status.

If no, it would be clearer to remove any mention CL_INVALID_VALUE from the documentation of binary_status.

@bashbaug
Copy link
Contributor

bashbaug commented Jul 9, 2024

Hi, good question! It looks like our implementation will return CL_INVALID_VALUE in this case but we will not update binary_status[i], though reading through the spec it's not clear to me whether this is the correct behavior either!

It would probably be good to hear how other vendors have interpreted the spec here.

Do you have a preference between your two options?

@LingMan
Copy link
Author

LingMan commented Jul 9, 2024

It would probably be good to hear how other vendors have interpreted the spec here.

Rusticl does set binary_status[i], but only on main and only since a few days ago. That MR is what spawned this question.

Do you have a preference between your two options?

My personal preference would be to not set binary_status[i]. It makes the implementation simpler and doesn't take any information away from the application. The application can't know which binary is invalid unless the implementation tells it, but the application can easily check which parameter it passed in is 0 / NULL.

Setting any binary_status[i] and expecting the application to read it also means all binaries have to be checked and all binary_status[i] have to be set before returning. At that point another question would be what the return value is when binaries[i] == NULL and binaries[j] is an invalid binary. CL_INVALID_VALUE or CL_INVALID_BINARY?

@bashbaug
Copy link
Contributor

bashbaug commented Jul 9, 2024

My personal preference would be to not set binary_status[i]. It makes the implementation simpler and doesn't take any information away from the application. The application can't know which binary is invalid unless the implementation tells it, but the application can easily check which parameter it passed in is 0 / NULL.

Cool, this aligns with the discussion in today's OpenCL teleconference, where we also observed that an application (or a debug layer) could check whether the size is zero or the binary is NULL, but cannot check whether a non-NULL binary is valid for a specific device. We didn't reach a conclusion today, but maybe we're getting close.

At that point another question would be what the return value is when binaries[i] == NULL and binaries[j] is an invalid binary. CL_INVALID_VALUE or CL_INVALID_BINARY?

When multiple errors occur we generally do not specificy which should be returned, so my first reaction is that it would be correct to return either error. I suspect most implementations would check for a NULL binary first and hence would hence return CL_INVALID_VALUE, but this wouldn't be guaranteed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cts-coverage The CTS needs to be extended
Projects
Status: Needs WG discussion
Development

No branches or pull requests

2 participants