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

Allow unconsumed inputs in fragment shaders #5531

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

Imberflur
Copy link
Contributor

@Imberflur Imberflur commented Apr 14, 2024

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:

  1. Pass info about vertex outputs when generating fragment inputs and add in the missing fields to the fragment input struct.
  2. 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 Imberflur force-pushed the allow-unconsumed-inputs branch 2 times, most recently from 1ed945e to f226525 Compare April 14, 2024 18:45
Copy link
Contributor Author

@Imberflur Imberflur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some questions and notes for reviewers.

naga/src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
naga/src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
wgpu-core/src/validation.rs Outdated Show resolved Hide resolved
wgpu-core/src/validation.rs Outdated Show resolved Hide resolved
tests/tests/regression/issue_3748.rs Outdated Show resolved Hide resolved
naga/tests/snapshots.rs Show resolved Hide resolved
@Imberflur Imberflur force-pushed the allow-unconsumed-inputs branch 2 times, most recently from c3caa2d to 0a04bf3 Compare April 15, 2024 01:14
@cwfitzgerald cwfitzgerald marked this pull request as ready for review April 17, 2024 05:31
@cwfitzgerald cwfitzgerald requested review from a team as code owners April 17, 2024 05:31
@cwfitzgerald
Copy link
Member

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

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tests/tests/regression/issue_3748.rs Outdated Show resolved Hide resolved
tests/tests/regression/issue_3748.rs Outdated Show resolved Hide resolved
naga/src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
wgpu-core/src/validation.rs Outdated Show resolved Hide resolved
@Imberflur Imberflur force-pushed the allow-unconsumed-inputs branch 3 times, most recently from 9ae8bb7 to 3f11d08 Compare April 22, 2024 01:30
@Imberflur Imberflur force-pushed the allow-unconsumed-inputs branch 2 times, most recently from 03f0bba to de5faaf Compare May 5, 2024 23:06
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@teoxoy teoxoy self-assigned this Jun 13, 2024
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Sorry for taking so long to take a look at it.

@teoxoy
Copy link
Member

teoxoy commented Jun 24, 2024

@Imberflur could you rebase the PR?

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.
@Imberflur
Copy link
Contributor Author

@teoxoy rebased!

@nical nical merged commit 3a68147 into gfx-rs:trunk Jul 4, 2024
25 checks passed
@Imberflur Imberflur deleted the allow-unconsumed-inputs branch July 4, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow disabling the StageError::InputNotConsumed error
4 participants