Skip to content

Shader input/output validation#705

Merged
bors[bot] merged 3 commits intogfx-rs:masterfrom
kvark:validation
Jun 7, 2020
Merged

Shader input/output validation#705
bors[bot] merged 3 commits intogfx-rs:masterfrom
kvark:validation

Conversation

@kvark
Copy link
Copy Markdown
Member

@kvark kvark commented Jun 7, 2020

Connections
Fixes #269
It still has a few gaps (like the storage_texture_format matching, bugs, optional validation), but the main logic is there. Anything else should come in smaller issues as a follow-up.

Description
The main goal of this PR is to validate:

  • vertex input stage against VS inputs
  • VS outputs against FS inputs
  • FS outputs against the pipeline attachment formats

I figured that WGPU_SHADER_VALIDATION environment is not a great path forward. It doesn't help engine authors, for example. So I'm switching it to just a boolean field in DeviceDescriptor. Hopefully, we'll remove it soon :)

Testing
Just running wgpu-rs examples - gfx-rs/wgpu-rs#354

Review notes: the commit in the middle just moves stuff around. I think it's easier to just review the first and the last commit, ignoring the middle.

@kvark kvark requested review from cwfitzgerald and grovesNL June 7, 2020 03:37
Copy link
Copy Markdown
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.

One comment, otherwise looks okay to me.

)
.unwrap();
validated_stages |= wgt::ShaderStage::FRAGMENT;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be referring to fragment stage this whole time, up until validated_stages == wgt::ShaderStage::VERTEX which seems to be talking to vertex. Either I misunderstand the code, or something is fishy

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The point here is not validating the fragment shader if the vertex shader wasn't validated, since we wouldn't know about FS inputs in this case.

Copy link
Copy Markdown
Member Author

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you for the quick review!

)
.unwrap();
validated_stages |= wgt::ShaderStage::FRAGMENT;
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The point here is not validating the fragment shader if the vertex shader wasn't validated, since we wouldn't know about FS inputs in this case.

@kvark
Copy link
Copy Markdown
Member Author

kvark commented Jun 7, 2020

Looks like we have no major concerns, let's proceed!
bors r=cwfitzgerald

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Jun 7, 2020

@bors bors Bot merged commit b76c848 into gfx-rs:master Jun 7, 2020
@kvark kvark deleted the validation branch June 7, 2020 14:46
bors Bot added a commit that referenced this pull request Jun 8, 2020
706: Return errors on create_render_pipeline r=cwfitzgerald a=kvark

This is a follow-up to #705 that switches `create_render_pipeline` to return `Result`.

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
bors Bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Jun 9, 2020
354: Update to wgpu with shader validation r=cwfitzgerald a=kvark

Depends on gfx-rs/wgpu#705

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
354: Update to wgpu with shader validation r=cwfitzgerald a=kvark

Depends on gfx-rs#705

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introspect the shader requirements for validation

2 participants