Workaround shader compiler bugs with degenerate switch statements

[Imberflur]

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]

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]

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.

[cwfitzgerald]

Wgpu side looks amazing, minus a nit

[Imberflur]

Let me know if/when conflicts need to be fixed.

[jimblandy]

@Imberflur Started reviewing this. My impression so far is that this is excellent work. I especially appreciate the execution tests and the docs.

[jimblandy]

@Imberflur Could you check out the docs I just pushed in 4c10a41 b185798?

[jimblandy]

Let me know if/when conflicts need to be fixed.

It seems like this needs a rebase.

edit: I did the rebase.

[Imberflur]

The new docs look great! Thanks for adding them.

I included one note for consideration.

P.S. The "forwarding" terminology is somewhat ad hoc, so feel free to suggest something else (like "deferring") if you have opinions there.

[Imberflur]

I'm somewhat curious about what conflicts came up in the rebase (in the interest of double checking them). But I can always re-run the rebase locally to find them.

[jimblandy]

I'm somewhat curious about what conflicts came up in the rebase (in the interest of double checking them). But I can always re-run the rebase locally to find them.

Only the absence of a cache field in the pipeline descriptors.

[jimblandy]

This looks great, and I think it's correct, but I'd like to see it simplified in two ways:

• Don't bother counting nesting depth; just always push/pop new entries on the stack. It took me a bit to realize that the depth counters were just an optimization, not something that actually affected the code generated.

• Eliminate ContinueCtx::next_id, let enter_switch take a &mut Namer argument, and just use the namer to generate unique names directly. It took me a while to be sure that the names you were generating couldn't conflict; if the code just uses the namer, then that question won't even arise.

  This means that Nesting::Switch will need to actually own the String. I think it will need to be an Rc<String>, both so that nested Switch variants can share it, and also so that exit_switch can return it regardless of whether it's been popped. ExitControl flow will need to hold a clone of the String.

[jimblandy]

I played with having the HLSL and GLSL writers just save the current nesting state in a local variable, and restore it on exit, so you could replace ContinueCtx::stack with a single Nesting value. But the API doesn't end up as nice as the one you've got, so I thought it would end up being more distracting in the end.

[jimblandy]

@Imberflur The only other suggestion I might have - and maybe this is just too hairy to be worth it - is that we might put a flag in Nesting::Switch to say whether it actually encountered any Continue statements that needed to be forwarded. If not, then there's no need to generate the check after the switch. Omitting those checks might make it easier for dumb drivers to see that the bool is truly unused.

[Imberflur]

just use the namer to generate unique names directly

Oh that sounds great! I didn't realize there was a mechanism for this.

[Imberflur]

@jimblandy I implemented suggestions:

• Removed nesting depth counter.
• Used Namer and Rc<String> for variable names.
• Omitted check generation if no continue statements are found.
• Rebased

[jimblandy]

Okay, I fiddled with the docs some more, and cleaned up a bit of the control flow, and I think this ready to land.

[Imberflur]

Thanks!