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

Update inter-stage interface validation to follow spec #5577

Open
4 tasks
Imberflur opened this issue Apr 22, 2024 · 0 comments
Open
4 tasks

Update inter-stage interface validation to follow spec #5577

Imberflur opened this issue Apr 22, 2024 · 0 comments
Labels
area: validation Issues related to validation, diagnostics, and error handling

Comments

@Imberflur
Copy link
Contributor

Imberflur commented Apr 22, 2024

https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-inter-stage-interfaces

I noticed a few inconsistencies with the spec when reading through the interface validation code in wgpu-core.

  • User-defined variables all count as 4 components towards the limit (not what ty.dim.num_components() returns).

    Each user-defined output of descriptor.vertex consumes 4 scalar components.

    For each user-defined input of descriptor.fragment there must be a user-defined output of descriptor.vertex that location, type, and interpolation of the input.

  • If the topology is "point-list" the component limit for vertex outputs needs to be reduced by 1.

    If descriptor.primitive.topology is "point-list":

    Decrement maxVertexShaderOutputComponents by 1.

  • For fragment inputs, the front_facing, sample_index, and sample_mask builtins all count as 1 component towards the limit.

  • There is now a maxInterStageShaderVariables (also mentioned in Further spec compliance #2170):

    The location of each user-defined output of descriptor.vertex must be < device.limits.maxInterStageShaderVariables.


I also had a couple questions on things aren't necessarily issues but weren't obvious to me when reading the code:

  • At the point where validation is done:
    Varying::Local { ref iv, .. } => iv.ty.dim.num_components(),
    are all Varying::Local variables "user-defined"? (in terms of how the spec uses "user-defined")
  • Matching types between stages is checked via frag_input_ty.is_subtype_of(vert_output_ty) (here). E.g. for a particular location this would allow vec4<f32> on the vertex side and f32 on the fragment side. Whether this is allowed isn't clear to me when looking at the line in the spec:

    For each user-defined input of descriptor.fragment there must be a user-defined output of descriptor.vertex that location, type, and interpolation of the input.

Imberflur added a commit to Imberflur/wgpu that referenced this issue Apr 22, 2024
* 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
Imberflur added a commit to Imberflur/wgpu that referenced this issue Apr 22, 2024
* 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
Imberflur added a commit to Imberflur/wgpu that referenced this issue Apr 22, 2024
* 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
@Wumpf Wumpf added the area: validation Issues related to validation, diagnostics, and error handling label Apr 22, 2024
Imberflur added a commit to Imberflur/wgpu that referenced this issue May 3, 2024
* 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
Imberflur added a commit to Imberflur/wgpu that referenced this issue May 5, 2024
* 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 added a commit to Imberflur/wgpu that referenced this issue Jul 4, 2024
* 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.
nical pushed a commit that referenced this issue Jul 4, 2024
* Allow unconsumed inputs in fragment shaders by removing them from vertex
outputs when generating HLSL.

Fixes #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.

* Address review

* 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
  #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: validation Issues related to validation, diagnostics, and error handling
Projects
Status: No status
Development

No branches or pull requests

2 participants