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

rephrase and correct the descriptions for clSetKernelExecInfo #1245

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bashbaug
Copy link
Contributor

@bashbaug bashbaug commented Aug 29, 2024

fixes #1152
fixes #1244

Rephrases and corrects the descriptions for multiple parts of clSetKernelExecInfo.

Removes the cumbersome "note" below the API description and moves the content into the API description instead. I don't think I lost any important content, but this would be a good area for a careful review.

Explicitly states that an empty set of SVM pointers for CL_KERNEL_EXEC_INFO_SVM_PTRS is valid.

@bashbaug
Copy link
Contributor Author

Depending how we resolve #1244 we may need to make additional clarifications.

@bashbaug
Copy link
Contributor Author

bashbaug commented Sep 3, 2024

Converting to draft to work out system SVM updates.

@bashbaug bashbaug marked this pull request as draft September 3, 2024 16:43
clarify that CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM does not
affect kernel arguments
@bashbaug
Copy link
Contributor Author

bashbaug commented Sep 4, 2024

This is ready for review again.

@bashbaug bashbaug marked this pull request as ready for review September 4, 2024 05:27
and the device does not support SVM, or if system pointers are passed as
arguments to a kernel and the device does not support fine-grain system SVM,
or if {CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM} is {CL_TRUE} and the device
does not support fine-grain system SVM.
Copy link
Contributor

@karolherbst karolherbst Sep 14, 2024

Choose a reason for hiding this comment

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

I'm not sure if the CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM part is really needed here. A client could use a kernel on multiple devices, some not supporting system SVM. Should we now require applications to flip CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM each time they enqueue the kernel on a different device?

It kinda feels more like an inconvenience, because for the runtime it's more or less meaningless as well. If an application uses a system SVM pointer on a device not supporting it indirectly, having CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM set to CL_TRUE or CL_FALSE doesn't really change anything about the abnormal behavior the client will see.

There is another aspect to this, by default CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM is CL_TRUE/CL_FALSE depending on the device. An application might set it to CL_FALSE for devices supporting system SVM to be nice to the runtime, but maybe they want to revert that, so now they'll need to toggle, which earlier they probably wouldn't had to.

I think it would probably make more sense to simply state, that CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM is assumed to be always CL_FALSE on devices the kernel gets enqueued not supporting system SVM or that this flag only has an impact on kernels for devices supporting system SVM.

Though to be fair, I doubt there is any applications doing that, but so far it just sounds like an inconvenience gets added for no real benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Full disclosure: I've never liked CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM. Having the default state dependent on the capabilities of the device always seemed especially problematic.

This is an interesting idea:

I think it would probably make more sense to simply state, that CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM is assumed to be always CL_FALSE on devices the kernel gets enqueued not supporting system SVM or that this flag only has an impact on kernels for devices supporting system SVM.

I think this could solve the default state problem as well, since then the default could be CL_TRUE if any device supports system SVM, and it'd just be ignored for devices that do not support system SVM.

If we're OK with this, I think I'd just need to make a few small edits to remove this error condition and to tidy up the CL_KERNEL_EXEC_INFO_SVM_FINE_GRAIN_SYSTEM description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants