-
Notifications
You must be signed in to change notification settings - Fork 844
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
Workaround shader compiler bugs with degenerate switch statements #5654
base: trunk
Are you sure you want to change the base?
Conversation
I guess this is one of the nested test cases I added. |
4c594bc
to
d8a33e9
Compare
d8a33e9
to
a90271e
Compare
Found minimal case #5658 and was able to avoid the issue here without affecting the aspects I was trying to test. |
631ee78
to
4666d96
Compare
4666d96
to
9b9630d
Compare
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.
Wgpu side looks amazing, minus a nit
to work around hlsl bug
Includes naga snapshot tests and a wgpu regression test
9b9630d
to
ce64106
Compare
Let me know if/when conflicts need to be fixed. |
Connections
Fixes #4514
Fixes #4485
Description
As discussed in #4485, FXC has a bug when handling switches with all cases leading to one body (it deletes the switch). It was also mentioned that some glsl consumers might not handle this scenario well either (but I don't know which ones that might be).
So we lower these to
do {} while(false);
loops in hlsl-out and glsl-out.While FXC already can't handle continue statements in switches (see #4485), I ended up including logic to properly forward continue statements to the outer loop here, since there would otherwise be regressions in glsl-out and maybe hlsl-out with DXC.
An alternative solution could be to switch on a known constant and have a dummy case with an empty body that is never hit. This is a bit less cautious in terms of avoiding potential compiler bugs since we are still switching on a constant. But the implementation would be simpler and the only issues I have observed so far with FXC are when all cases lead to a single body.
Testing
Some of these scenarios are already covered by existing naga snapshot tests, I extended
naga/tests/in/control-flow.wgsl
with a few more cases.I also included some regression tests for the linked issues that will execute the shaders and assert on their output. Additionally, a new mechanism is introduced into the test framework for forcing the usage of FXC when on the Dx12 backend.
I ran
cargo xtask test
on a windows machine and a separate linux machine.Checklist
cargo fmt
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.