Allow unconsumed inputs in fragment shaders #5531

Imberflur · 2024-04-14T18:20:43Z

By removing them from vertex outputs when generating HLSL.

Add naga::back::hlsl::FragmentEntryPoint for providing information about the fragment entry point when generating vertex entry points via naga::back::hlsl::Writer::write. Vertex outputs not consumed by the fragment entry point are omitted in the final output struct.
Add naga snapshot test for this new feature,
Remove Features::SHADER_UNUSED_VERTEX_OUTPUT, StageError::InputNotConsumed, and associated validation logic.
Make wgpu dx12 backend pass fragment shader info when generating vertex HLSL.
Add wgpu regression test for allowing unconsumed inputs.

Connections
Fixes #3748

Description
My case affected by this is passing in SPIRV from shaderc which can optimize out unused inputs from the fragment shader. The shaders are dynamically configured with a pre-processor based on various settings from the user so it can be hard to predict when certain inter-stage variables are unused, and my attempt at configuring out particular outputs from vertex shaders quickly became overly verbose.

As noted in the linked issue, this validation was originally in the WebGPU spec but was removed. The validation was necessary for generating HLSL with matching inter-stage interfaces but we can work around that by adjusting the HLSL generation to account for any unconsumed inputs.

After investigating this I found two potential ways to account for this in the generated HLSL:

Pass info about vertex outputs when generating fragment inputs and add in the missing fields to the fragment input struct.
Pass info about fragment inputs when generating vertex outputs and omit unconsumed fields from the vertex output struct.

I went with the second option since it seemed simpler to implement than generating new fields and nicely removed unnecessary work passing unused values (although I assume drivers can probably optimize this out).

Note: This is a breaking change.

Testing
I added a test to for whether the wgpu validation logic now allows unconsumed inputs and a naga snapshot test for removing the unconsumed vertex outputs when generating HLSL.

~~I have not tested on a windows machine with the dx12 backend!~~ So here are some testing TODOs:

Run cargo xtask test on machine with dx12.
Cherry-pick this to branch I'm currently using for veloren and test if it works there.

Checklist

Run cargo fmt.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- --target wasm32-unknown-emscripten
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

Imberflur

Here are some questions and notes for reviewers.

Imberflur · 2024-04-14T18:52:01Z

naga/src/back/hlsl/writer.rs

+    // TODO: log error if binding is none?
    // technically, this should always be `Some`
    binding: Option<crate::Binding>,


I added a note to remind myself here and in several places below.

Should this log an error? Are there cases where we actually expect None?

I can:

Remove these notes if logging isn't desired.

Otherwise, add some logging (and remove notes).

Or, remove notes and defer question/implementation to a separate issue/PR

I'm adding a debug_assert!(member.binding.is_some()) into write_interface_struct

Imberflur · 2024-04-14T18:53:15Z

naga/src/back/hlsl/writer.rs

                TypeInner::Struct { ref members, .. } => {
+                    // TODO: what about nested structs? Is that possible? Maybe try an unwrap on
+                    // the binding?
                    for member in members.iter() {


I feel like I saw something about it being possible to nest structs here. Is this possible? Should we open an issue for handling that? I could also attempt handling it in this PR.

You cannot nest - should probably note this in the code

From the spec

Each user-defined input and output must have an explicitly specified IO location. Each structure member in the entry point IO must be one of either a built-in value (see § 12.3.1.1 Built-in Inputs and Outputs), or assigned a location.

And looks like this is checked when validating the Module by VaryingContext::validate?

This also implies that binding is always Some... I'm tempted to add at least a debug assertion for this.

I added a note

wgpu-core/src/validation.rs

tests/tests/regression/issue_3748.rs

Imberflur · 2024-04-14T19:11:37Z

naga/tests/snapshots.rs

+    // Uses separate wgsl files to make sure the tested code doesn't accidentally rely on
+    // the fragment entry point being from the same parsed content (e.g. accidentally using the
+    // wrong `Module` when looking up info). We also don't just create a module from the same file
+    // twice since everything would probably be stored behind the same keys.
+    let (input, mut module) = load_and_parse("unconsumed_vertex_outputs_vert");
+    let (frag_input, mut frag_module) = load_and_parse("unconsumed_vertex_outputs_frag");


Adding this was a bit awkward since it didn't align very well with the existing tests. I needed a new parameter on check_targets. It looks like I could have added a new field to the Parameters struct to specify the fragment entry point. However, I wanted to specifically test the case of the fragment shader being from a separate Module.

cwfitzgerald · 2024-04-17T05:32:16Z

Great to see work here! Marking un-draft so it gets added to the review queue

cwfitzgerald

wgpu side looks fine, added some comments some of the naga things

tests/tests/regression/issue_3748.rs

cwfitzgerald · 2024-04-17T20:01:56Z

naga/src/back/hlsl/writer.rs

                TypeInner::Struct { ref members, .. } => {
+                    // TODO: what about nested structs? Is that possible? Maybe try an unwrap on
+                    // the binding?
                    for member in members.iter() {


You cannot nest - should probably note this in the code

From the spec

Each user-defined input and output must have an explicitly specified IO location. Each structure member in the entry point IO must be one of either a built-in value (see § 12.3.1.1 Built-in Inputs and Outputs), or assigned a location.

wgpu-core/src/validation.rs

outputs when generating HLSL. Fixes gfx-rs#3748 * Add naga::back::hlsl::FragmentEntryPoint for providing information about the fragment entry point when generating vertex entry points via naga::back::hlsl::Writer::write. Vertex outputs not consumed by the fragment entry point are omitted in the final output struct. * Add naga snapshot test for this new feature, * Remove Features::SHADER_UNUSED_VERTEX_OUTPUT, StageError::InputNotConsumed, and associated validation logic. * Make wgpu dx12 backend pass fragment shader info when generating vertex HLSL. * Add wgpu regression test for allowing unconsumed inputs.

* Add note that nesting structs for the inter-stage interface can't happen. * Remove new TODO notes (some addressed and some transferred to an issue gfx-rs#5577) * Changed issue that regression test refers to 3748 -> 5553 * Add debug_assert that binding.is_some() in hlsl writer * Fix typos caught in CI Also, fix compiling snapshot test when hlsl-out feature is not enabled.

cwfitzgerald

Just clearing from my review queue, need someone from naga to look at this

Imberflur force-pushed the allow-unconsumed-inputs branch 2 times, most recently from 1ed945e to f226525 Compare April 14, 2024 18:45

Imberflur commented Apr 14, 2024

View reviewed changes

Imberflur force-pushed the allow-unconsumed-inputs branch 2 times, most recently from c3caa2d to 0a04bf3 Compare April 15, 2024 01:14

cwfitzgerald marked this pull request as ready for review April 17, 2024 05:31

cwfitzgerald requested review from a team as code owners April 17, 2024 05:31

cwfitzgerald approved these changes Apr 17, 2024

View reviewed changes

Imberflur force-pushed the allow-unconsumed-inputs branch 3 times, most recently from 9ae8bb7 to 3f11d08 Compare April 22, 2024 01:30

Imberflur requested a review from cwfitzgerald April 22, 2024 01:54

Imberflur force-pushed the allow-unconsumed-inputs branch from 3f11d08 to 03f0bba Compare May 3, 2024 03:25

Imberflur force-pushed the allow-unconsumed-inputs branch from 03f0bba to de5faaf Compare May 5, 2024 23:06

cwfitzgerald approved these changes May 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow unconsumed inputs in fragment shaders #5531

Allow unconsumed inputs in fragment shaders #5531

Imberflur commented Apr 14, 2024 •

edited

Imberflur left a comment

Imberflur Apr 14, 2024 •

edited

Imberflur Apr 21, 2024

Imberflur Apr 14, 2024

cwfitzgerald Apr 17, 2024

Imberflur Apr 21, 2024

Imberflur Apr 21, 2024

Imberflur Apr 22, 2024

Imberflur Apr 14, 2024

cwfitzgerald commented Apr 17, 2024

cwfitzgerald left a comment

cwfitzgerald Apr 17, 2024

cwfitzgerald left a comment

Allow unconsumed inputs in fragment shaders #5531

Are you sure you want to change the base?

Allow unconsumed inputs in fragment shaders #5531

Conversation

Imberflur commented Apr 14, 2024 • edited

Imberflur left a comment

Choose a reason for hiding this comment

Imberflur Apr 14, 2024 • edited

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

cwfitzgerald commented Apr 17, 2024

cwfitzgerald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwfitzgerald left a comment

Choose a reason for hiding this comment

Imberflur commented Apr 14, 2024 •

edited

Imberflur Apr 14, 2024 •

edited