OpenCL-Docs
OpenCL-Docs copied to clipboard
KhronosGroup
Function pointer naming
Currently, function pointers definitions are placed inside <command> and require full-on parsing to properly interpret, because there are no tags to separate parameters and return type.
Also, the cl_arm_printf extension requires its own function pointer type, but there is nowhere to define it in XML.
So, I've added <type category="function"> tags. I've also merged function pointer types with the same parameters under a common name. Including cases where original spacing differs:
clEnqueueSVMFreeARM:
void * svm_pointers[], void *user_data
clEnqueueSVMFree:
void* svm_pointers[], void* user_data
Nice PR, it makes writing a binding generator easier. Maybe functions should be added as OpenCL version/extension require's too?
@bashbaug, what do you think about require tags? I can add that in this PR, or a separate one, I'm not sure if that would be extra complexity here...
what do you think about require tags?
Sorry for the very slow reply. I think adding the function pointer types is going to be the more contentious issue, since they aren't described in the spec currently.
If we decide to add the function pointer types then I think it would make sense to add the appropriate "require" tags to associate them with the right OpenCL version, but again, that's a big "if".
How confident are we that introducing the function pointer types isn't going to cause a compatibility issue?
Could we make this change as part of the next OpenCL version? This way an application would only see an issue when moving to headers for the newer version. Or, does this just add complexity with no real benefit?
How confident are we that introducing the function pointer types isn't going to cause a compatibility issue?
Can't C header generator emit the same header after these changes? It can translate functions as typedefs and then use them in command declarations, but it can also write out function prototype for every argument, like it's done currently. The only difference is going to be in spaces, like pointed out in the PR description.
Added function types to <require> tags.
Usage of function types per API function:
cl_enqueue_svm_free_callback clEnqueueSVMFreeARM
cl_enqueue_svm_free_callback clEnqueueSVMFree
cl_create_context_callback clCreateContext
cl_create_context_callback clCreateContextFromType
cl_context_destructor_callback clSetContextDestructorCallback
cl_mem_object_destructor_callback clSetMemObjectDestructorCallback
cl_mem_object_destructor_callback clSetMemObjectDestructorAPPLE
cl_program_callback clBuildProgram
cl_program_callback clCompileProgram
cl_program_callback clLinkProgram
cl_program_callback clSetProgramReleaseCallback
cl_event_callback clSetEventCallback
cl_enqueue_native_kernel_callback clEnqueueNativeKernel
It seems this last commit broke generators, and I might as well try to do as @RocketRide9 described.
Can't C header generator emit the same header after these changes?
This is admittedly somewhat philosophical, but IMHO the XML file should be a faithful representation of the specification, which should faithfully be described by the headers, so it'd be odd for the three to be out-of-sync.
I took the liberty to create a proposal defining those types in the headers from the specification. https://github.com/KhronosGroup/OpenCL-Headers/pull/294 I kept the callbacks type name in the same format as the current function types, but I am open to suggestion/discussion. Does this proposal help?
We decided to take a look at adding a form of this to the specification. For discussion purposes, here is how Vulkan does it:
<comment>The PFN_vk*Function types are used by VkAllocationCallbacks below</comment>
<type category="funcpointer">typedef void (VKAPI_PTR *<name>PFN_vkInternalAllocationNotification</name>)(
<type>void</type>* pUserData,
<type>size_t</type> size,
<type>VkInternalAllocationType</type> allocationType,
<type>VkSystemAllocationScope</type> allocationScope);</type>
<type category="funcpointer">typedef void (VKAPI_PTR *<name>PFN_vkInternalFreeNotification</name>)(
<type>void</type>* pUserData,
<type>size_t</type> size,
<type>VkInternalAllocationType</type> allocationType,
<type>VkSystemAllocationScope</type> allocationScope);</type>
<type category="funcpointer">typedef void* (VKAPI_PTR *<name>PFN_vkReallocationFunction</name>)(
<type>void</type>* pUserData,
<type>void</type>* pOriginal,
<type>size_t</type> size,
<type>size_t</type> alignment,
<type>VkSystemAllocationScope</type> allocationScope);</type>
<type category="funcpointer">typedef void* (VKAPI_PTR *<name>PFN_vkAllocationFunction</name>)(
<type>void</type>* pUserData,
<type>size_t</type> size,
<type>size_t</type> alignment,
<type>VkSystemAllocationScope</type> allocationScope);</type>
<type category="funcpointer">typedef void (VKAPI_PTR *<name>PFN_vkFreeFunction</name>)(
<type>void</type>* pUserData,
<type>void</type>* pMemory);</type>
Added function types to
<require>tags.Usage of function types per API function:
cl_enqueue_svm_free_callback clEnqueueSVMFreeARM cl_enqueue_svm_free_callback clEnqueueSVMFree cl_create_context_callback clCreateContext cl_create_context_callback clCreateContextFromType cl_context_destructor_callback clSetContextDestructorCallback cl_mem_object_destructor_callback clSetMemObjectDestructorCallback cl_mem_object_destructor_callback clSetMemObjectDestructorAPPLE cl_program_callback clBuildProgram cl_program_callback clCompileProgram cl_program_callback clLinkProgram cl_program_callback clSetProgramReleaseCallback cl_event_callback clSetEventCallback cl_enqueue_native_kernel_callback clEnqueueNativeKernelIt seems this last commit broke generators, and I might as well try to do as @RocketRide9 described.
I like the fact that you tried to factor the callbacks. Though I think some type safety might be useful when the intent of the callback is different. If we factor the callback, then I think the callback function should be clear in the name, I am very bad at naming things, but here are a few counter proposal to get the ball rolling:
- cl_free_svm_callback: clEnqueueSVMFreeARM, clEnqueueSVMFree
- cl_context_error_callback: clCreateContext, clCreateContextFromType
- cl_context_destruction_callback: clSetContextDestructorCallback
- cl_mem_object_destruction_callback: clSetMemObjectDestructorCallback, clSetMemObjectDestructorAPPLE
- cl_program_build_completion_callback: clBuildProgram
- cl_program_compile_completion_callback: clCompileProgram
- cl_program_link_completion_callback: clLinkProgram
- cl_program_destruction_callback: clSetProgramReleaseCallback
- cl_event_status_change_callback: clSetEventCallback
- cl_native_kernel: clEnqueueNativeKernel
My reasoning for not factoring the different program callbacks is that you would most probably not want to use these interchangeably, since they have different intent.
Agree with almost everything (and assume you meant _callback at the end of cl_native_kernel), and I still trust your naming sense more than mine))
But for now, I'm not convinced that we need to separate function types with the same parameters.
Especially between clBuildProgram, clCompileProgram, and clLinkProgram. These functions are very similar in their use in practice, and I can imagine a helper function for all of them - why make that harder?
More generally, as I see it now, the main reason for strongly typing enums is the IDE suggestions, with preventing the passing of incorrect values being secondary. And it's hard to imagine either reason being relevant for function types.
(I'm very open to discussion about this)
P.S. There is also cl_printf_callback, which I somehow missed in my list above, in case you also want to suggest its rename.
In the cases when a callback is used by a core API and an extension API we have an additional wrinkle to consider: if an extension was added to "backport" a feature to a prior version of OpenCL, it would be undesirable for the extension to rely on a function pointer type that is only defined for a newer version of OpenCL. Therefore, we would want the core function pointer type to be defined for all versions of OpenCL, not just when the core API is added. Not the end of the world, but this feels a little weird.
We should also prepare for the eventuality where an extension adds a callback and later gets prompted to a core feature.
Given these issues, would it be easier to have a specific type for a core API callback vs. an extension API callback?
We could also do something like this, which we've done a few times in the past, but it's always felt a little ugly:
#if !defined(CL_VERSION_2_1)
/* defined in CL.h for OpenCL 2.1 and newer */
typedef cl_uint cl_kernel_sub_group_info;
#endif /* !defined(CL_VERSION_2_1) */
Turns out funcpointer type category was already prepared, though never implemented, so I was able to fix tests by just renaming my category to that:
https://github.com/KhronosGroup/OpenCL-Docs/blob/d1eb4558941185a1e0cf3d5d34498370e068bba9/scripts/reg.py#L1129
Though now it creates a few broken files in .\gen\api\funcpointers, seemingly taking only const strings from <type> tags.
But for now, I'm not convinced that we need to separate function types with the same parameters. Especially between
clBuildProgram,clCompileProgram, andclLinkProgram. These functions are very similar in their use in practice, and I can imagine a helper function for all of them - why make that harder? More generally, as I see it now, the main reason for strongly typing enums is the IDE suggestions, with preventing the passing of incorrect values being secondary. And it's hard to imagine either reason being relevant for function types. (I'm very open to discussion about this)
I definitely think that the callback type for clSetProgramReleaseCallback is needed, it is most probably a programming error if a callback one wrote for getting the notification your build is finished is used as a release notification. For the other three, my opinion is not as strong obviously.
Added callback for clSetProgramReleaseCallback and the guards for callbacks defined in both core and ext.
@bashbaug, as I understand, we are now waiting on a decision: Do we want to have generators output inline callbacks as before, or to change the API to a (mostly) compatible one with separately defined callbacks, as in the sister PR https://github.com/KhronosGroup/OpenCL-Headers/pull/294