-
Notifications
You must be signed in to change notification settings - Fork 781
Add VK_KHR_pipeline_binary sample #1418
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
base: main
Are you sure you want to change the base?
Conversation
Introduce a new Vulkan sample demonstrating VK_KHR_pipeline_binary usage. It includes querying pipeline keys, capturing pipeline binaries, and managing them explicitly. The sample is accompanied by documentation, CMake lists, shaders, and updates to the Vulkan framework mapping.
- Add detailed logging for pipeline binary operations. - Introduce UI overlay in `PipelineBinary` to display aggregated log information. - General code clean-up and formatting adjustments.
gary-sweet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can only really say that this builds ok as we don't currently support pipeline binaries.
asuessenbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice little sample. Just a few comments...
# Conflicts: # framework/vulkan_type_mapping.h # samples/extensions/README.adoc
…etrics Add interactive GUI to demonstrate pipeline recreation from scratch vs. from cached binaries with timing measurements. Implement binary persistence (save/load to disk) and display performance statistics including speedup calculations. Update documentation to reflect new interactive features and workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a new volk version?
Shouldn't that be done by a separate PR?
Replace C-style string formatting (snprintf with char buffers) with std::format for cleaner, more modern C++ code. Use vk::to_string for VkResult enum values instead of casting to int. Consolidate duplicate LOGI/LOGW calls and log_text_ updates into single operations per message.
|
You might need to |
|
I see a bunch of compilation errors: |
|
hmm... Issue here. std format was introduced in C++20. I don't think we've updated this project to require C++20 yet. @asuessenbach are you okay if I revert back to the original version? |
|
We did update to C++20 in march 2025, see #1257 |
|
ahh... right, I forgot thanks! Okay, fixing CI to allow for format. |
Configure clang-tidy to use C++20 standard and reformat command for readability. Fix include order in pipeline_binary.cpp to follow conventions (standard headers alphabetically).
…lchain path Install g++-13 to provide libstdc++ headers and configure clang-tidy to use GCC 13 toolchain via --gcc-toolchain flag to resolve C++20 standard library compatibility issues.
Remove Docker container dependency and install steps. Detect and use available run-clang-tidy version (18, 17, or default) from the system. Remove GCC 13 toolchain configuration as it's no longer needed.
Install libxcb1-dev to satisfy CMake dependencies and remove GLFW_BUILD_X11=OFF flag to restore default X11 support.
|
Alright, the clang-tidy check in CI finally passed. I might recommend we take this PR before we take any other PRs that use C++20 so the CI has a chance to stay green. |
|
I'm still seeing one build error sadly: |
|
That's what I was talking about with regards to do we want to use C++20 everywhere. format is the C++20 header. You might need to see if you can't get a newer STL. clang-tidy had the same issues which is what I fixed with the CI check.yml file if you want to review it. |
Hmm. Well we (as in the GPU group) don't have any control over the toolchain that is required to be used for our set-top boxes unfortunately. We're currently forced to use gcc12. If that's the problem here we won't be able to build anything at all after this. |
|
Let's discuss on the next Samples call. We can targeted only rely upon things that also exist in gcc12 in my opinion as I don't think we get anything but syntactic sugar with std::format instead of what I had previously; also I really don't want to exclude any devices or people that are actively helping if we can help it :) |
Agreed. We literally wouldn't be able to run anything if we're forced down that route. |
|
For this sample, I'll go ahead and revert the std format requirement (if that's okay @asuessenbach). I'll do that in the morning though, it's my 1am. |
My advice - go to bed! |
|
I think, I don't get that: we're on C++20 and gcc12 supports that nicely. But it doesn't have that |
I really don't like that |
This is what AI has to say about it (believe as much of it as you dare): |
|
We do include fmt as a formatting library that's already used in the framework and samples. Why not use that instead of std::format? |
Switch from C++20 std::format to fmt library for broader compiler compatibility. Fix formatting and indentation inconsistencies introduced in previous refactoring.
|
Excellent suggestion Sascha. fmt seems to be a good direct replacement. |
|
More build errors I'm afraid: |
|
Ah... I missed one... simply replacing std to fmt will fix it. I'll push a fix just a sec. |
|
Thanks, this does now build for me, and correctly reports that we don't support the extension. |
Description
Introduce a new Vulkan sample demonstrating VK_KHR_pipeline_binary usage. It includes querying pipeline keys, capturing pipeline binaries, and managing them explicitly. The sample is accompanied by documentation, CMake lists, shaders, and updates to the Vulkan framework mapping.
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batchcommand line argument to make sure all samples still work properlySample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: