Switch statement improvements #2126

teoxoy · 2022-11-15T13:24:15Z

fixes #2031
fixes #2128

ErichDonGubler

Disclaiming that I'm still inexperienced with compiler work, and that I don't have any actual rights with this repo...LGTM (!), minus questions.

ErichDonGubler · 2022-11-15T18:52:00Z

src/back/wgsl/writer.rs

+                let mut new_case = true;
+                for case in cases {
+                    if case.fall_through && !case.body.is_empty() {
+                        // TODO: we could do the same workaround as we did for the HLSL backend


question: I know that in wgpu, cwfitzgerald requests that TODOs be either resolved or converted to issues. I see that there are lots of other TODOs in the repo, currently; does naga maintainership take a stance on "WIP"-ish comments?

This hasn't been brought up as far as I'm aware. It would be nice but I think we should do a round of converting all TODOs to issues first (which has been on my mind for a while but not a priority currently).

We shouldn't introduce any new TODO comments to Naga. The issue tracker is a much better tool for keeping track of these sorts of plans and ideas, so please file an issue for this. (Instead of a "TODO" comment, a link to an issue is fine, if you think it would be helpful to future readers.)

That's fair, filed https://github.com/gfx-rs/naga/issues/2166.

tests/wgsl-errors.rs

tests/out/glsl/control-flow.main.Compute.glsl

ErichDonGubler · 2022-11-15T20:07:54Z

src/back/wgsl/writer.rs

-                        match case.value {
-                            crate::SwitchValue::Integer(value) => {
-                                writeln!(self.out, "{}case {}{}: {{", l2, value, type_postfix)?;
+                let mut new_case = true;


question: It's a bummer that we're not removing fall_through, too; the complexity. From our sync conversation, you mentioned that it wasn't feasible to remove at this time, because IR needs to keep it for other front ends (IIRC?). Is there an issue to track this?

That but also the fact that we still translate a switch with multiple cases with the same body as a sequence of switch cases with no body and fall_through = true ending in a case with the body and fall_through = false.
i.e.

switch(v) { case 0, 1: { p = 3; } }

V

SwitchCase { value: 0, fall_through = true, body: {} }, SwitchCase { value: 1, fall_through = false, body: { p = 3 } }

I'll add some more docs on the Switch statement.

idea(non-blocking): Maybe we could change SwitchCase model to remove fall_through and make body an Option? Concretely:

/// <snip> struct SwitchCase { // … /// When [`None`], a subsequent case should contain the body associated with this case. body: Option<Block>, // replaces `fall_through` and current definition of `body` }

A one-off enum might also serve well here, but I'm less certain about that.

The other frontends still make use of fall_through with non empty bodies though.

This code that decides when to write "case" is fine, but it does seem like the IR is not making the writer's life easy. We might be able to improve the IR, but that should be a separate PR.

Suggestion: Would it work to do something like this?

// Gather cases into groups that all execute the same block. // Only the last case in each group has a non-empty body. for case_group in cases.split_inclusive(|case| !(case.fall_through && case.body.is_empty())) { write!("case "); for value in case_group.iter().map(|case| case.value) { write!("{}, ", value); } writeln!(":"); write_block(case_group.last().body); }

(I'm just making this up, please check my logic carefully!)

Granted, this would need to be changed when we implement fallthrough, but I think if we tradesplit_inclusive for .enumerate().filter() and then gather up all the consecutive bodies we need to concatenate, we could keep this as a loop whose iterations correspond to emitted "case" statements, not IR cases, which is what I think makes it clearer.

This code that decides when to write "case" is fine, but it does seem like the IR is not making the writer's life easy. We might be able to improve the IR, but that should be a separate PR.

I thought about this but I could not come up with a satisfactory change because we still need to support "real" fall-through for GLSL and SPV.

Granted, this would need to be changed when we implement fallthrough, but I think if we trade split_inclusive for .enumerate().filter() and then gather up all the consecutive bodies we need to concatenate, we could keep this as a loop whose iterations correspond to emitted "case" statements, not IR cases, which is what I think makes it clearer.

I don't see how .enumerate().filter() would work since we still need to emit the case values of the fall_through cases. As far as I can tell the only way to do this would be to allocate a new structure and have one pass restructuring the data then another to write it which I don't think is really worth it.

also add support for default to be used with other case selectors

jimblandy

This looks great! Some minor points.

jimblandy · 2022-12-08T16:41:28Z

src/back/wgsl/writer.rs

+                let mut new_case = true;
+                for case in cases {
+                    if case.fall_through && !case.body.is_empty() {
+                        // TODO: we could do the same workaround as we did for the HLSL backend


We shouldn't introduce any new TODO comments to Naga. The issue tracker is a much better tool for keeping track of these sorts of plans and ideas, so please file an issue for this. (Instead of a "TODO" comment, a link to an issue is fine, if you think it would be helpful to future readers.)

jimblandy · 2022-12-08T16:56:14Z

src/back/wgsl/writer.rs

-                        match case.value {
-                            crate::SwitchValue::Integer(value) => {
-                                writeln!(self.out, "{}case {}{}: {{", l2, value, type_postfix)?;
+                let mut new_case = true;


This code that decides when to write "case" is fine, but it does seem like the IR is not making the writer's life easy. We might be able to improve the IR, but that should be a separate PR.

Suggestion: Would it work to do something like this?

// Gather cases into groups that all execute the same block. // Only the last case in each group has a non-empty body. for case_group in cases.split_inclusive(|case| !(case.fall_through && case.body.is_empty())) { write!("case "); for value in case_group.iter().map(|case| case.value) { write!("{}, ", value); } writeln!(":"); write_block(case_group.last().body); }

(I'm just making this up, please check my logic carefully!)

Granted, this would need to be changed when we implement fallthrough, but I think if we tradesplit_inclusive for .enumerate().filter() and then gather up all the consecutive bodies we need to concatenate, we could keep this as a loop whose iterations correspond to emitted "case" statements, not IR cases, which is what I think makes it clearer.

src/front/wgsl/mod.rs

src/back/glsl/mod.rs

src/back/hlsl/writer.rs

src/back/spv/block.rs

tests/out/msl/control-flow.msl

jimblandy · 2022-12-09T06:36:14Z

Rebased and tweaked the Switch docs.

ErichDonGubler approved these changes Nov 16, 2022

View reviewed changes

teoxoy added 8 commits December 7, 2022 17:31

[wgsl-in] use parse_block in more places

e55823d

[wgsl] remove fallthrough statement

5e99640

also add support for default to be used with other case selectors

[msl-out] omit extra switch case blocks where possible

d1948e3

[hlsl-out] omit extra switch case blocks where possible

69342d1

[spv-out] fix switch cases after default not being output

5c87a4e

[spv-out] omit extra switch case blocks where possible

1880404

[glsl-out] introduce a new block for switch cases

833ce7d

[doc] explain how case clauses with multiple selectors are supported

e9f817d

jimblandy requested changes Dec 9, 2022

View reviewed changes

jimblandy force-pushed the switch-improvements branch from e0185d1 to e9f817d Compare December 9, 2022 06:35

[doc] add more switch docs

cbf84b9

teoxoy requested a review from jimblandy December 9, 2022 11:38

jimblandy approved these changes Dec 9, 2022

View reviewed changes

jimblandy merged commit 5a1f43d into gfx-rs:master Dec 9, 2022

teoxoy deleted the switch-improvements branch December 9, 2022 22:06

teoxoy mentioned this pull request Jan 31, 2023

Default not working with another clause gfx-rs/wgpu#3441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch statement improvements #2126

Switch statement improvements #2126

teoxoy commented Nov 15, 2022 •

edited

Loading

ErichDonGubler left a comment •

edited

Loading

ErichDonGubler Nov 15, 2022

teoxoy Nov 17, 2022

jimblandy Dec 8, 2022

teoxoy Dec 9, 2022

ErichDonGubler Nov 15, 2022

teoxoy Nov 17, 2022

ErichDonGubler Nov 17, 2022

teoxoy Nov 17, 2022 •

edited

Loading

jimblandy Dec 8, 2022

teoxoy Dec 9, 2022

jimblandy left a comment

jimblandy Dec 8, 2022

jimblandy Dec 8, 2022

jimblandy commented Dec 9, 2022

Switch statement improvements #2126

Switch statement improvements #2126

Conversation

teoxoy commented Nov 15, 2022 • edited Loading

ErichDonGubler left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teoxoy Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblandy commented Dec 9, 2022

teoxoy commented Nov 15, 2022 •

edited

Loading

ErichDonGubler left a comment •

edited

Loading

teoxoy Nov 17, 2022 •

edited

Loading