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

Add pass for generating API methods with a function pointer parameter #303

Merged
merged 3 commits into from
Jun 27, 2021

Conversation

serenity4
Copy link
Contributor

@serenity4 serenity4 commented Jun 26, 2021

Following from JuliaGPU/VulkanCore.jl#44, here is a pass for adding extra methods to API functions that ccall into libraries, exposing a function pointer argument for short-circuiting the library symbol lookup.

I tested it on VulkanCore.jl, both with use_ccall_macro turned on and off, and it seems to work as expected (update PR to come after this is merged). The test infrastructure did not seem to provide a handy way to test the behavior of this specific pass.

I made the inclusion of the pass to be a general option named "add_fptr_methods", feel free to suggest another name if that is not clear.

Copy link
Contributor Author

@serenity4 serenity4 left a comment

Choose a reason for hiding this comment

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

Left a few comments regarding potential guarantees that we might have for the expressions contained in FunctionProto nodes.

src/generator/passes.jl Outdated Show resolved Hide resolved
src/generator/passes.jl Outdated Show resolved Hide resolved
@serenity4
Copy link
Contributor Author

serenity4 commented Jun 26, 2021

To see the behavior, feel free to re-run the generator in VulkanCore.jl with this branch and setting a general option "add_fptr_methods" to true. Here is an overview:

# with use_ccall_macro off

function vkDestroyDeferredOperationKHR(device, operation, pAllocator)
    ccall((:vkDestroyDeferredOperationKHR, libvulkan), Cvoid, (VkDevice, VkDeferredOperationKHR, Ptr{VkAllocationCallbacks}), device, operation, pAllocator)
end

function vkDestroyDeferredOperationKHR(device, operation, pAllocator, fptr)
    ccall(fptr, Cvoid, (VkDevice, VkDeferredOperationKHR, Ptr{VkAllocationCallbacks}), device, operation, pAllocator)
end

# with use_ccall_macro on

function vkDestroyDeferredOperationKHR(device, operation, pAllocator)
    @ccall libvulkan.vkDestroyDeferredOperationKHR(device::VkDevice, operation::VkDeferredOperationKHR, pAllocator::Ptr{VkAllocationCallbacks})::Cvoid
end

function vkDestroyDeferredOperationKHR(device, operation, pAllocator, fptr)
    @ccall ($fptr)(device::VkDevice, operation::VkDeferredOperationKHR, pAllocator::Ptr{VkAllocationCallbacks})::Cvoid
end

Copy link
Member

@Gnimuc Gnimuc left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature! I re-run the generator script for VulkanCore.jl and the results look good: Gnimuc/GeneratorScripts@627b412

src/generator/passes.jl Outdated Show resolved Hide resolved
src/generator/passes.jl Outdated Show resolved Hide resolved
src/generator/passes.jl Outdated Show resolved Hide resolved
src/generator/context.jl Outdated Show resolved Hide resolved
@Gnimuc
Copy link
Member

Gnimuc commented Jun 27, 2021

I made the inclusion of the pass to be a general option named "add_fptr_methods", feel free to suggest another name if that is not clear.

Looks like VulkanCore.jl is the only user of this option, so let's keep it secret for now and bikeshedding the name in another issue.

@serenity4
Copy link
Contributor Author

Thanks for the quick review. I made the suggested changes, it's a bit simpler now.

@Gnimuc Gnimuc merged commit d691ad7 into JuliaInterop:master Jun 27, 2021
@Gnimuc
Copy link
Member

Gnimuc commented Jun 27, 2021

Thanks for the contribution!

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.

2 participants