-
Notifications
You must be signed in to change notification settings - Fork 1k
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 unchecked shader modules to also skip (naga's) uniformity checks #3175
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3175 +/- ##
==========================================
- Coverage 64.77% 64.75% -0.02%
==========================================
Files 81 81
Lines 38724 38756 +32
==========================================
+ Hits 25084 25098 +14
- Misses 13640 13658 +18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, some comments
wgpu/src/lib.rs
Outdated
/// the caller responsibility to pass a shader which doesn't perform any of these | ||
/// operations. | ||
/// | ||
/// This is equivalent to `create_shader_module` effect on web. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually equivalent. With disabling bounds checks, it doesn't actually change the validity of the shader. This method makes otherwise invalid shaders valid. I think we need a proper feature for this. Maybe UNCHECKED_SHADERS
to cover any future validation restrictions too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was coming at this from 'web=webgpu', which should have uniformity analysis, forgetting the 'web=webgl' case. Thanks for the reminder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is more that, when calling *_unchecked, it doesn't matter if there are bounds checks in the shader or not, so the user doesn't need to know if eliding bounds checks are supported.
When calling *_non_uniform
, it does matter if eliding uniformity analysis is supported, because some shaders won't compile. Because of this, we need some way to inform the user if eliding uniformity analysis is supported on the device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly? I'd hope use of this API would be relatively rare.
More precisely, I think that the non-uniform part of this API is kind of inherently tied to working around naga, with knowledge of the code it emits to the backend. It's unclear to me what cases are valid wgsl if they don't pass uniformity analysis. That is, if you have a version of your code which uses the fact that uniformity analysis can be disabled, and another one which doesn't, you should always use the one which doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the workgroupBroadcast proposal is accepted (and implemented in naga), then that would be my preferred way to avoid triggering uniformity analysis errors or warnings, and then I would not use this API. However, it might still make sense as a short term solution.
Closing this for now. We will revisit if and when naga's uniformity check is improved such that we need the workaround, but at the moment we can just make use of the fact naga doesn't actually check uniformity for the scenario we need. |
Checklist
cargo clippy
Ran but couldn't find any non-pre-existing errors. Didn't do a complete diff as that felt like a waste of time.
RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown
if applicable.I think not applicable, but ran anyway - no warnings. Please clarify what "if applicable" means here.
Connections
Modelled after #1978, using the same struct this already created.
Fixes #3135
The changes might be removable after gpuweb/gpuweb#3479 is resolved and filters through
Description
When using shaders with
wgpu
, it may be necessary to work around gpuweb/gpuweb#3479.To allow this,
Device::create_shader_module_unchecked
has gained a parameter for configuration. This allows This disablingValidationFlags::CONTROL_FLOW_UNIFORMITY
when used, but may allow maintaining bounds checks.Testing
I don't believe that we should expect much use of this API; it should primarily be used for exploratory work.
TODO, awaiting results from @raphlinus