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

Introspect the shader requirements for validation #269

Closed
3 of 6 tasks
kvark opened this issue Aug 7, 2019 · 11 comments · Fixed by #705
Closed
3 of 6 tasks

Introspect the shader requirements for validation #269

kvark opened this issue Aug 7, 2019 · 11 comments · Fixed by #705
Labels
area: validation Issues related to validation, diagnostics, and error handling

Comments

@kvark
Copy link
Member

kvark commented Aug 7, 2019

We need some way to ask the shader modules for what their expected bindings are, vertex attributes, etc. Currently, it's easy to get the shader mismatched with the pipeline layout, and we aren't validating it.

Edit: here is a tentative plan, and it involves Naga:

  • make it so a ShaderModel contains an Option<naga::Module>. The goal is to make it a no-option at some point
  • implement the logic of validation at pipeline creation, based on those modules. If the module isn't present, skip the validation.
  • when accepting SPIR-V, try parsing it into the module, but don't panic if it fails

Follow-up plan:

  • finish render pipeline creation validation
  • when accepting WGSL, we have to both parse it, and produce the SPIR-V to feed gfx-rs
  • make naga's SPIR-V front-end to work
@fintelia
Copy link
Contributor

fintelia commented Jan 6, 2020

I've never used it, but this spirq-rs crate was posted on r/rust_gamedev not long ago, and looks like it might be well suited for doing the necessary shader introspection.

@kvark
Copy link
Member Author

kvark commented Jan 6, 2020

Right, I believe @swiftcoder is looking into it.

@GabrielMajeri
Copy link
Contributor

Is @swiftcoder still working on this, or is it up for contributing?

@kvark
Copy link
Member Author

kvark commented Apr 24, 2020

Doesn't look like @swiftcoder is doing this, consider it available for contribution!

The situation has changed though. We don't want to invest into SPIR-Q, instead we'd want to integrate naga and dog-food it.

The first stage is having naga as an optional dependency, and using its SPIR-V front-end to load the naga::Module (which is our IR). It will not be used to produce anything, but instead - to analyze shader requirements and validate them against the pipeline. We'd allow the SPIR-V front-end to fail, in which case we'd issue a warning and continue without validation (while things settle down).

@GabrielMajeri
Copy link
Contributor

Now that we no longer have wgpu-native and its examples in this repository, how could I go around testing the shader validation works?

Would adding a test that directly calls wgpu-core functions to create a device and loads a .spv from disk work?

@kvark
Copy link
Member Author

kvark commented Apr 25, 2020

The tests you are considering is more than just a shader, it's the whole pipeline. We could just defer this to the point where we could run wgpu-native against WebGPU CTS, which covers the validation purposes.

Alternatively, once #289 is implemented, we'll be able to have small internal tests based on the deserialization of the work.

@GabrielMajeri
Copy link
Contributor

I've looked into this, but I don't know exactly where the introspection & validation would take place.

We are given the raw SPIR-V code in device_create_shader_module. However, the validation would take place when creating computer/render pipelines, at which point we no longer have acces to the original code.

Should we store the SPIR-V code somewhere? Or do we expose this validation functionality as helper functions?

@kvark
Copy link
Member Author

kvark commented May 4, 2020

The natural spot for validation is create_render_pipeline and create_compute_pipeline.
What happens on create_shader_module is just parsing the shader and ensuring it's self-consistent to start with (i.e. a valid SPIR-V/WGSL).

Edit: yes, feel free to add more metadata on our struct ShaderModule, such as SPIR-V or some processed version of it.

@kvark
Copy link
Member Author

kvark commented May 6, 2020

I've put our plan up in the body of the issue.

The last bit here is not required, but I think it makes sense, because it's fairly easy to build on top of the other steps, and our WGSL front-end is pretty mature, unlike the SPIR-V front-end. The only blocker here is that SPIR-V backend is still not merged - gfx-rs/naga#46 (cc @Napokue ).

So for now, we can't do (4), but we can do the other 3 points in preparation to it. But most importantly, we can start having that validation code based on naga::Module.

bors bot added a commit that referenced this issue May 9, 2020
641: Add optional SPIR-V shader validation r=kvark a=GabrielMajeri

This PR adds some basic validation for SPIR-V shaders when creating pipelines. Starts work towards #269.

Currently, I'm marking this as a draft because `naga` isn't mature enough to be able to parse shaders from the `wgpu-rs` examples.

For example:
- Trying to run `hello-triangle` from `wgpu-rs` results in the following error:
`Failed to parse shader SPIR-V code: UnsupportedInstruction(Function, Variable)`
- For `hello-compute` it is:
`Failed to parse shader SPIR-V code: UnsupportedInstruction(Type, TypeBool)`

Co-authored-by: Gabriel Majeri <[email protected]>
@Yamakaky
Copy link

Is validating if a buffer has the right BufferStage setting a part of this issue or should I create a new one?

@kvark
Copy link
Member Author

kvark commented May 27, 2020

@Yamakaky buffer having the right usage flags is unrelated to this issue. It's about shader requirements only.

bors bot added a commit that referenced this issue Jun 6, 2020
704: Pipeline layout validation r=cwfitzgerald a=kvark

**Connections**
Implements a solid part of #269
Starts converting the function to return results, related to #638
cc @GabrielMajeri 

**Description**
This change matches shader bindings against the pipeline layout. It's *mostly* complete, minus some bugs and not handling the `storage_texture_format` properly.

The risk here is that Naga reflection may have bugs, or our validation may have bugs, and we don't want to break the user content while this is in flux. So the PR introduces an internal `WGPU_SHADER_VALIDATION` environment variable. Switching it to "0" skips Naga shader parsing completely and allows the users to unsafely use the API.

Another aspect of the PR is that some of the functions now return `Result`. The way I see us proceeding is that any errors that we don't expect users to handle should result in panics when `wgpu` is used natively (i.e. not from a browser). These panics would happen in the "direct" backend of wgpu-rs (as well as in wgpu-native), but the `Result` would not be exposed to wgpu-rs, so that it matches the Web behavior.

At the same time, browser implementations (Gecko and Servo) will check the result on their GPU process and implement the WebGPU error model accordingly. This means `wgpu-core` can be super Rusty and safe.

**Testing**
Running on wgpu-rs examples. Most of them fail to get parsed by Naga, but `boids` succeeds and passes validation 🎉 

Co-authored-by: Dzmitry Malyshau <[email protected]>
@bors bors bot closed this as completed in b76c848 Jun 7, 2020
@bors bors bot closed this as completed in #705 Jun 7, 2020
@kvark kvark unpinned this issue Jun 8, 2020
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
None yet
Development

Successfully merging a pull request may close this issue.

4 participants