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 C Interface for Optimizer #5030

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

daniel-story
Copy link
Contributor

This PR implements a C interface for the Optimizer class. It follows the same pattern established by the existing C interfaces for other classes in SPIRV-Tools.

Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

This looks good.
But it needs at least one test to cover the new functionality.

Please add a test/opt/c_interface_test.cpp

@dneto0
Copy link
Collaborator

dneto0 commented Dec 20, 2022

And remember to update both
CMakeLists.txt
Android.mk

@s-perron
Copy link
Collaborator

I agree. Looks good. It just needs a test. Than the CI will let you know if you missed any build files.

@s-perron
Copy link
Collaborator

@daniel-story Are you still working on this?

@daniel-story
Copy link
Contributor Author

Apologies for the lack of movement on this.

Yes, I am still intending to work on this, but more time sensitive work has been taking precedence. It was less time-consuming to propose the PR in its current form as the C interface code was already written and being used in our fork, but I'll need to find time to implement the test code.

As an aside, looking over this again I think it may also make sense to make two small tweaks to the spvOptimizerRun() parameters:

  • Change optimized_binary to a pointer to a spv_binary (obviously itself a pointer to a spv_binary_t) so that the function can return an internally newed spv_binary which can be freed by the caller using spvBinaryDestroy() like the other spv_binary-returning functions (for example spvTextToBinary())
  • Potentially change original_binary to an unsigned int* and size_t make it more straightforward for callers to use with SPIR-V that came from outside the SPIRV-Tools ecosystem (for example from glslang), and to match spvBinaryToText()

I feel pretty strongly in favor of the first change, and more in favor than not about the second. Do you have any thoughts?

Assuming you're on board with these, I'll try to make both changes and push them along with the tests sometime within the next 2 weeks.

@s-perron
Copy link
Collaborator

Change optimized_binary to a pointer to a spv_binary (obviously itself a pointer to a spv_binary_t) so that the function can return an internally newed spv_binary which can be freed by the caller using spvBinaryDestroy() like the other spv_binary-returning functions (for example spvTextToBinary())

For consistency, I think that is a good idea.

Potentially change original_binary to an unsigned int* and size_t make it more straightforward for callers to use with SPIR-V that came from outside the SPIRV-Tools ecosystem (for example from glslang), and to match spvBinaryToText()

Sure.

@daniel-story
Copy link
Contributor Author

I’ve added a set of tests for the newly added functionality. I also updated the CMakeLists.txt and verified that a full test run passes successfully.

Regarding the Android tests, I couldn’t find anywhere to add these tests to the Android.mk miles as none of the Android.mk files in the project include seem to include any of the tests currently. I don’t think it’s realistic to expect this PR to port all the tests in SPIRV-Tools to Android, not just because it seems very out of scope but also because I’m not set up to build or run anything on Android.

Please let me know if you have any more comments, or if I missed or misunderstood something. Otherwise, this PR should be ready to merge.

s-perron
s-perron previously approved these changes Jan 27, 2023
test/opt/c_interface_test.cpp Outdated Show resolved Hide resolved
s-perron
s-perron previously approved these changes Jan 28, 2023
@s-perron
Copy link
Collaborator

s-perron commented Feb 1, 2023

The windows 2019 bots probably failed because you need to rebase. I'll merge anyway. Other windows tests passed.

@s-perron s-perron merged commit 0994ca4 into KhronosGroup:main Feb 1, 2023
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.

4 participants