Switch statement improvements

[teoxoy]

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]

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?

[teoxoy]

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).

[jimblandy]

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.)

[teoxoy]

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

[ErichDonGubler]

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?

[teoxoy]

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.

[ErichDonGubler]

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.

[teoxoy]

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

[jimblandy]

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.

[teoxoy]

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.

[jimblandy]

This looks great! Some minor points.

[jimblandy]

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]

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.

[jimblandy]

Rebased and tweaked the Switch docs.