Move shader analysis to Vulkano crate, make available for runtime shaders #1747

Rua · 2021-11-08T14:47:04Z

- **Breaking** Vulkano-shaders no longer generates a `Shader` struct, but instead provides `load` as a standalone function that returns `Arc<ShaderModule>` directly.
- **Breaking** Vulkano-shaders no longer generates a function for each entry point. Entry points are now retrieved using the `entry_point` method of `ShaderModule`.
- **Breaking** The `shader` module is moved to the crate root, and `spirv` is now a submodule of it.
- **Breaking** `ShaderModule::new` is renamed to `from_bytes` to match the existing `from_words`.
- **Breaking** `ShaderModule` now parses and analyses the shader provided to it on construction, checks whether the shader is compatible with the device, and gathers data such as descriptor sets and input/output interfaces directly from its code. To provide this data yourself as before, and avoid parsing, you can use the `_with_data` constructors.
- **Breaking** `ComputeEntryPoint` and `GraphicsEntryPoint` are removed, as are the functions on `ShaderModule` that generate them. Instead, you can look up an entry point using the `entry_point` method.

- Added support for additional shader types in `ShaderStage`, `ShaderStages` and `PipelineStages`.

Fixes #1558, #1673. This moves most of the shader analysis code from Vulkano-shaders to the main Vulkano crate, so that it can be used for runtime-loaded shaders as well. ShaderModule will now analyse the code given to it by default, but it's still possible to provide the data manually if you want. This is done by Vulkano-shaders to avoid having to parse and analyse the code each time the program is run, so you don't have to pay a performance penalty for using Vulkano-shaders.

The new code also checks compatibility between the shader (SPIR-V capabilities and extensions) and the device. For this, I added more code to autogen that generates checks from vk.xml, which already contains information about device requirements for each SPIR-V capability and extension.

The rest of the changes are mainly rearranging and changes of APIs to facilitate this. Shader entry points can now be retrieved at runtime via a string, based on the data collected from the shader. Vulkano-shaders generates its code a little differently.

…able for runtime shaders

Eliah-Lakhin · 2021-11-09T03:20:27Z

examples/src/bin/shader-types-sharing.rs

@@ -102,7 +102,7 @@ fn main() {
                // by default and the macro by default producing unique
                // structs(`MultSpecializationConstants`, `AddSpecializationConstants`)
                shared_constants: true,


If you change this flag to false, it will generate multSpecializationConstants/addSpecializationConstants(is it?), which is also violates naming convention.

Ah, I missed that bit. I can convert the name to CamelCase using the heck crate that is already used by autogen.

Eliah-Lakhin · 2021-11-09T03:22:57Z

examples/src/bin/teapot/main.rs

@@ -301,8 +307,8 @@ fn main() {
 /// This method is called once during initialization, then again whenever the window is resized
 fn window_size_dependent_setup(
    device: Arc<Device>,
-    vs: &vs::Shader,
-    fs: &fs::Shader,
+    vs: Arc<ShaderModule>,


Maybe pass &ShaderModule here to avoid vs/fs cloning?

Ah, that was a leftover from an earlier experiment I did with how the entry points should work, that I ended up abandoning. I'll fix it.

Eliah-Lakhin · 2021-11-09T03:30:08Z

vulkano-shaders/src/codegen.rs

@@ -775,6 +343,19 @@ mod tests {
            .collect()
    }

+    #[test]
+    fn test() {


Can you give more descriptive name to this test, please? In case it fails, it would be easier to find the test code.

Eliah-Lakhin · 2021-11-09T03:40:26Z

vulkano/autogen/spirv_parse.rs

        .operand_kinds
        .iter()
-        .filter(|operand_kind| operand_kind.category == "BitEnum")
+        .cloned()


Here and in similar places perhaps cloning and twice collecting can be avoided by turning functional-style call chaining into imperative for-loop. The entire code would look shorter and will cost lesser performance and memory.

Do you mean having a mut kinds_dedup and then pushing elements into it? I don't know if that would be any faster, and I've always specifically avoided that style in favour of collect. My impression is that Rust is quite smart when it comes to optimising iterators, but comparing the assembly output or doing a benchmark is probably the only way to know for sure.

As for the cloning, I suppose that could be avoided, I'll see what I can do.

Eliah-Lakhin · 2021-11-09T03:44:56Z

vulkano/src/pipeline/compute_pipeline.rs

@@ -49,8 +50,8 @@ impl ComputePipeline {
    /// to add dynamic buffers or immutable samplers.
    pub fn new<Css, F>(
        device: Arc<Device>,
-        shader: &ComputeEntryPoint,
-        spec_constants: &Css,
+        shader: EntryPoint,


Why owned value instead of reference?

I noticed while working that GraphicsPipelineBuilder took an owned GraphicsEntryPoint while ComputePipeline took a borrowed &ComputeEntryPoint. I removed the borrow to make them both the same. They could both borrow instead, but I think that would be problematic for GraphicsPipelineBuilder, and for performance a borrow just adds one layer of indirection.

Eliah-Lakhin · 2021-11-09T03:47:40Z

vulkano/src/shader/mod.rs

@@ -7,15 +7,15 @@
 // notice may not be copied, modified, or distributed except
 // according to those terms.

-//! Stage of a graphics pipeline.
+//! A program that is run on the GPU.


More broadly speaking on the Graphics Device which could be in theory a CPU-backed emulator of Vulkan :)

Good point, fixed!

Eliah-Lakhin · 2021-11-09T03:58:28Z

@Rua Thank you for your hard work! It's amazing!

I only did a brief code review at this moment, left a few comments to discuss and maybe to fix. I will take a closer look later and do some tests.

In addition to that I have a concern about the generated entry-point reusability. Previously the user could load the module, and then reuse it's main entry point every time they create a pipeline. WIll they be able to do the same in the new implementation, or the entry point generation in place is unavoidable?

Rua · 2021-11-09T09:48:14Z

EntryPoint is not very expensive to generate. The entry_point method will borrow the ShaderModule, look up the name in a HashMap, and generate a new CString (which admittedly could be done in the pipeline builder, but performance wise it doesn't matter where it's done).

Eliah-Lakhin · 2021-11-13T10:13:54Z

@Rua Can you fix merging conflicts, please?

…untime-shader-parsing

Rua · 2021-11-13T10:17:53Z

Done.

Eliah-Lakhin · 2021-11-13T12:33:19Z

@Rua I adapted my project's code to these changes, but I'm receiving the following error:

SpirvExtensionNotSupported {
             extension: "SPV_EXT_descriptor_indexing",
             reason: RequirementsNotMet(
                 [
                     "Vulkan API version 1.2",
                     "device extension `ext_descriptor_indexing`",
                 ],
             ),
         }

There was no such error before, and it worked fine. If I raise the Instance version to 1.2 the error is gone, but what I don't understand is that it works regardless to the Vulkan version specified in the vulkano_shaders macro(i.e. it works with 1.1 and 1.2 both).

Rua · 2021-11-13T12:37:00Z

Do you have the ext_descriptor_indexing extension enabled on your device? That's actually a requirement in the spec, listed here and in vk.xml. I guess that Vulkano-shaders did not previously check for this, but the PR is behaving correctly.

Eliah-Lakhin · 2021-11-13T12:48:02Z

@Rua I just tried to enabled it(for Vulkan 1.1). And it asked to enable "khr_maintenance3"(ExtensionRestrictionNotMet(ExtensionRestrictionError { extension: "ext_descriptor_indexing", restriction: RequiresDeviceExtension("khr_maintenance3") })). So I did it too. Now the error is:

FeatureRestrictionNotMet(
             FeatureRestrictionError {
                 feature: "descriptor_indexing",
                 restriction: RequiredByExtension(
                     "ext_descriptor_indexing",
                 ),
             },
         )

I guess that Vulkano-shaders did not previously check for this, but the PR is behaving correctly.

How so? I suppose, the Instance/Device of Vulkan 1.2 shouldn't let the user load the shader that requires Vulkan 1.2 and/or it's extensions, and explicitly specifies version 1.1.

Rua · 2021-11-13T12:55:42Z

And if you enable the feature too, what happens then? The feature requirement is also according to spec; some extensions require a feature to be enabled when the extension is enabled. ext_descriptor_indexing requires the feature descriptor_indexing, you can see it in the Vulkano documentation: https://docs.rs/vulkano/0.26.0/vulkano/device/struct.Features.html#structfield.descriptor_indexing.

How so? I suppose, the Instance/Device of Vulkan 1.2 shouldn't let the user load the shader that requires Vulkan 1.2 and/or it's extensions, and explicitly specifies version 1.1.

It's not really a matter of the shader version itself as far as I can tell. Rather, the shader is compiled with certain SPIR-V capabilities and extensions enabled, and those in turn require matching features and extensions on the Vulkan device. If you look at the SPIR-V assembly code of your shader, you will see the OpCapability and OpExtension instructions. Vulkano sees those when it parses the shader and checks the device to make sure the proper support for them is enabled. Those instructions are in turn generated by shaderc based on the glsl code you've written.

As for the khr_maintenance3 requirement, that's actually fixed by #1748.

Eliah-Lakhin · 2021-11-13T13:43:37Z

@Rua Sorry, what do you mean by enabling the feature?

Rua · 2021-11-13T13:45:06Z

When you create a device, you pass in a Features struct. You enable a feature by setting the appropriate member of the struct to true.

Eliah-Lakhin · 2021-11-13T14:58:34Z

@Rua Got it. Now it says that the feature is not supported. It looks very strange, because all of it works with Vulkan 1.2 and without explicitly enabling of these features and extensions.

Eliah-Lakhin · 2021-11-13T14:58:58Z

Anyway, I don't think this is a blocker for the PR merging.

Rua · 2021-11-13T15:07:19Z

I think I see where the problem lies. The descriptor_indexing feature must indeed be enabled if the extension is enabled:

If ppEnabledExtensions contains "VK_EXT_descriptor_indexing" and the pNext chain includes a VkPhysicalDeviceVulkan12Features structure, then VkPhysicalDeviceVulkan12Features::descriptorIndexing must be VK_TRUE

However, VkPhysicalDeviceVulkan12Features is a Vulkan 1.2 structure, so it's not available on Vulkan 1.1 and therefore this VUID will never apply. The logic for checking this doesn't take the version into account, so that's a bug that still needs fixing.

Eliah-Lakhin · 2021-11-13T15:08:35Z

Understood. ok, I thought there is something with my gpu driver.

Anyway, PR is merged. Thanks for all the fixes and explanations!

Move shader parsing and information extraction to Vulkano, make avail…

9cc9d3f

…able for runtime shaders

Rua mentioned this pull request Nov 8, 2021

Raytracing flags in ShaderStages #1746

Closed

Rua added 2 commits November 8, 2021 15:52

Small change

3c069c9

Add new pipeline stages to match shader stages

92154c8

Eliah-Lakhin reviewed Nov 9, 2021

View reviewed changes

Eliah-Lakhin requested changes Nov 9, 2021

View reviewed changes

Eliah-Lakhin added the PR: In review. Comments to address. Maintainer started review process, and put additional questions and comments to be addressed. label Nov 9, 2021

Fix review issues

8333540

Merge branch 'master' of https://github.com/vulkano-rs/vulkano into r…

5bdaec5

…untime-shader-parsing

Eliah-Lakhin removed the PR: In review. Comments to address. Maintainer started review process, and put additional questions and comments to be addressed. label Nov 13, 2021

Eliah-Lakhin merged commit c6959aa into vulkano-rs:master Nov 13, 2021

Eliah-Lakhin added a commit that referenced this pull request Nov 13, 2021

#1747 Changelog update

1db775b

Rua mentioned this pull request Nov 13, 2021

Check for features that must be enabled with an extension doesn't take version into account #1750

Closed

incertia mentioned this pull request Nov 14, 2021

Make it possible to load shader bytecode with identical entry point names for different execution models #1752

Closed

Rua mentioned this pull request Nov 14, 2021

Two fixes for extension requirements #1748

Merged

Rua deleted the runtime-shader-parsing branch February 14, 2022 10:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move shader analysis to Vulkano crate, make available for runtime shaders #1747

Move shader analysis to Vulkano crate, make available for runtime shaders #1747

Rua commented Nov 8, 2021 •

edited

Loading

Eliah-Lakhin Nov 9, 2021

Rua Nov 9, 2021

Eliah-Lakhin Nov 9, 2021

Rua Nov 9, 2021

Eliah-Lakhin Nov 9, 2021

Rua Nov 9, 2021

Eliah-Lakhin Nov 9, 2021

Rua Nov 9, 2021

Eliah-Lakhin Nov 9, 2021

Rua Nov 9, 2021

Eliah-Lakhin Nov 9, 2021

Rua Nov 9, 2021

Eliah-Lakhin commented Nov 9, 2021 •

edited

Loading

Rua commented Nov 9, 2021

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021 •

edited

Loading

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021 •

edited

Loading

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Move shader analysis to Vulkano crate, make available for runtime shaders #1747

Move shader analysis to Vulkano crate, make available for runtime shaders #1747

Conversation

Rua commented Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eliah-Lakhin commented Nov 9, 2021 • edited Loading

Rua commented Nov 9, 2021

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021 • edited Loading

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021 • edited Loading

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 13, 2021

Eliah-Lakhin commented Nov 13, 2021

Rua commented Nov 8, 2021 •

edited

Loading

Eliah-Lakhin commented Nov 9, 2021 •

edited

Loading

Eliah-Lakhin commented Nov 13, 2021 •

edited

Loading

Rua commented Nov 13, 2021 •

edited

Loading