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

Workaround shader compiler bugs with degenerate switch statements #5654

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

Imberflur
Copy link
Contributor

@Imberflur Imberflur commented May 3, 2024

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

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Imberflur Imberflur requested review from a team as code owners May 3, 2024 06:07
@Imberflur
Copy link
Contributor Author

Imberflur commented May 3, 2024

stderr:
error: line 127: Case construct that targets '74[%74]' has invalid branch to block '55[%55]' (not another case construct, corresponding merge, outer loop merge or outer loop continue)
  %74 = OpLabel

I guess this is one of the nested test cases I added.

@Imberflur
Copy link
Contributor Author

stderr:
error: line 127: Case construct that targets '74[%74]' has invalid branch to block '55[%55]' (not another case construct, corresponding merge, outer loop merge or outer loop continue)
  %74 = OpLabel

I guess this is one of the nested test cases I added.

Found minimal case #5658 and was able to avoid the issue here without affecting the aspects I was trying to test.

@Imberflur Imberflur force-pushed the degenerate-switches branch 2 times, most recently from 631ee78 to 4666d96 Compare May 4, 2024 07:36
Copy link
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.

Wgpu side looks amazing, minus a nit

tests/src/init.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants