Function pointer naming

[SunSerega]

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

[vdualb]

Nice PR, it makes writing a binding generator easier. Maybe functions should be added as OpenCL version/extension require's too?

[CLAassistant]

All committers have signed the CLA.

[SunSerega]

@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...

[bashbaug]

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?

[vdualb]

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.

[SunSerega]

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.

[bashbaug]

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.

[Kerilk]

I took the liberty to create a proposal defining those types in the headers from the specification.
KhronosGroup/OpenCL-Headers#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?

[Kerilk]

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>

[Kerilk]

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.

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.

[SunSerega]

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.

[bashbaug]

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) */

[SunSerega]

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:

OpenCL-Docs/scripts/reg.py

Line 1129 in d1eb455

"funcpointer": {},

Though now it creates a few broken files in .\gen\api\funcpointers, seemingly taking only const strings from <type> tags.

[Kerilk]

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)

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.

[SunSerega]

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 KhronosGroup/OpenCL-Headers#294