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

Added support for custom SpecializedConstants and removed KomputeWorkgroup class #151

Merged
merged 20 commits into from
Feb 15, 2021

Conversation

axsaucedo
Copy link
Member

@axsaucedo axsaucedo commented Feb 13, 2021

Fixes:

Outstanding:

  • Current implementation is complex mainly to allow for float and uint32_T (and other mixed types), but the code would be massively simplified if always restricted to just std::vector where all spec constants are the same
    • Currently implementation uses a lot of malloc/free with raw memory so the value may not be worth for the maintenance and risks added
    • Another alternative is to introduce std::variant but it wouldn't remove the requirement for raw memory construction and management on the top-level container

After further exploration around other examples online that show how people use specialization constants (and even push constants) it's quite common for users to have mainly unit32_t and float, however for the initial implementation it may be easier to just limit users to input float values. This would also be possible to explore via #2, however it seems there hasn't really been much request for uint32_t, given that most of the use-cases are for compute it seems that float would be appropriate. One thing to explore is the performance impact for users that want to do float to int convesion inside shaders.

For explicitness this is the previous implementation that would've initially allowed for floats and ints, but also coudl've extended to more complex types https://github.com/EthicalML/vulkan-kompute/blob/7126cc47ffeb9770661447689763a2d602824ae6/src/include/kompute/Algorithm.hpp#L17-L112

  • Still requires adding python bindings

Done - I have added python bindings and removed the string functions

  • Further tests

Done - I added tests for constants and workgroups

  • Refactor to make simpler (rename to kp::Const)

This is now done, the refactor simplified to just floats, and now there is kp::Constants and kp::Workgroup

@axsaucedo axsaucedo added enhancement New feature or request python c++ labels Feb 13, 2021
@axsaucedo axsaucedo merged commit b9e0b5e into master Feb 15, 2021
@axsaucedo axsaucedo changed the title Added support for custom SpecializedConstants Added support for custom SpecializedConstants and removed KomputeWorkgroup class Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ enhancement New feature or request python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover all Python & C++ tests in CI
1 participant