Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Apply specialization to shader reflection #2329

Merged
merged 8 commits into from
Sep 14, 2023

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Sep 12, 2023

Changelog:

- - Specialization constants are now provided with a `HashMap` containing `SpecializationConstant` enum values. The `SpecializationConstants` trait is removed, and `vulkano_shaders` no longer generates structs for specialization constants.
+ - Specialization constants are now provided by calling `ShaderModule::specialize` with a `HashMap` containing `SpecializationConstant` enum values. This produces a `SpecializedShaderModule` value, which you can then create an `EntryPoint` from.
+ - The `SpecializationConstants` trait is removed, and `vulkano_shaders` no longer generates structs for specialization constants.
### Bugs fixed
- Specialization constants are now applied to the reflected SPIR-V code before any other reflection is performed.

Shader specialization has been moved to its own type, SpecializedShaderModule. You create this from a regular ShaderModule by calling with_specialization, and it has methods to create an entry point. The old entry point methods that were directly on ShaderModule are still there, but they are now simply shortcuts for calling with_specialization with no specialization constants, and then calling the corresponding entry point method of SpecializedShaderModule.

When you create a SpecializedShaderModule, a replacement pass is made through the reflected SPIR-V module, replacing all specialization constants with regular constants, applying the provided specialization constants where applicable. OpSpecConstantOp instructions are also evaluated in this pass, which was by far the most complex part of this PR, and also the most likely to have bugs I missed.

CHANGELOG.md Show resolved Hide resolved
vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
vulkano/src/shader/mod.rs Show resolved Hide resolved
@marc0246
Copy link
Contributor

marc0246 commented Sep 14, 2023

This doesn't fix #1956. The issue also isn't a bug, so the label is incorrect. As I commented, the issue is that they are trying to use vulkano-shaders, which generates Rust types at compile-time, with an array that is runtime-sized, which is impossible. From what I can tell vulkano-shaders is working exactly as intended there.

@Rua
Copy link
Contributor Author

Rua commented Sep 14, 2023

Good point, I had missed that the issue was with Vulkano-shaders specifically. Should we just close it as wontfix then?

@marc0246
Copy link
Contributor

Question is whether they would find vulkano-shaders that doesn't generate Rust types useful.

vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
@marc0246
Copy link
Contributor

Looks good, thanks!

@marc0246 marc0246 merged commit 3c49541 into vulkano-rs:master Sep 14, 2023
marc0246 added a commit that referenced this pull request Sep 14, 2023
@Rua Rua deleted the shader-specialization branch October 25, 2023 14:24
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
* Apply specialization to shader reflection

* Remove redundant method

* Remove all the SpecId decorations too

* Don't unnecessarily collect the instructions

* Replace decoration groups with individual decorations

* Rename with_specialization

* Missed renames

* Remove the Arcs
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants