gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.15k stars 888 forks source link

Naga should map `break if` 1:1 to the backend (i.e. `do`-`while` for GLSL/HLSL/MSL). #4560

Open eddyb opened 1 year ago

eddyb commented 1 year ago

Quoting myself from https://github.com/gfx-rs/wgpu/issues/4558:

I do not understand why break if turns into that kind of conditional break.

AIUI it was only added to WGSL to represent SPIR-V conditional backedges. And SPIR-V itself only has conditional backedges to encode do-while loops. So that's the natural translation.

That is, loop { ... continuing { ... break if !(select(false, true, _e23)); } } should turn into do { ... } while(!!(_e23 ? true : false)); - without any overcomplicated propagation across the backedge.

I believe Naga already maps break if 1:1 to WGSL (as expected) and to SPIR-V (as conditional backedges).

So only the C-like backends (GLSL/HLSL/MSL) remain, and they should all allow a 1:1 mapping via do-while.

jimblandy commented 1 year ago

The story is a little more complicated than that. WGSL continuing blocks with break if statements do two things:

This is needed specifically to support SPIR-V input, because SPIR-V allows (almost) arbitrary control flow in the continuing block. (This makes it easier to generate SPIR-V that represents a function call inlined into a C-style for loop's third expression.)

But I think this means that back ends cannot always generate do-while loops for Naga Loop statements. If the loop's body block is exited by a Continue statement, and the continuing block can't be represented as a single expression, then that can't be represented as a do-while. Modulo bugs, of course, the more awkward transform we do now handles all the cases.

Of course, it wouldn't be surprising if compilers downstream from Naga generated better code if we could hand them a do-while loop when applicable. That would be an optimization, then.

eddyb commented 1 year ago

Ah, I see, I was focusing on continuing {...} without control-flow (or with control-flow that could be handled by ?: expressions alone).

There... is another way, but it's probably worse in some respects.

The insight comes from inlining SPIR-V (which you mention), and how ... (click to open rant) ... it's actually incredibly annoying because SPIR-V is not "structured" as much as it is conservatively approximating "structurally reconvergent" (useful definitions of "structured" tend to be *sufficient* for inlining by substitution - i.e. `break`/`continue`/`return` are "unstructured non-local exits" which break up structured constructs like `if`-`else`/`switch`/`loop`, they just happen to be "structurally reconvergent" which is what drivers want - note that Rust-style arbitrary `break`-with-label would be too).

What spirv-opt does to inline a function is... wrap it in switch(0) { default: ... } and replace returns with breaks.

So, this is one approach that doesn't require propagating a condition across the loop backedge:

loop {
    // ...
    if a { break; }
    // ...
    if b { continue; }
    // ...
    continuing {
        // ...
        break if c;
    }
}

could be turned into:

do {
    bool loop_break = false;
    switch(0) {
        default: // loop body
            // ...
            if (a) { loop_break = true; break; }
            // ...
            if (b) { break; }
            // ...
    }
    if (loop_break) break;

    // continuing block
    // ...
} while(!c);

It should be relatively simple transform, and there's a chance it will optimize better than something that looks like it needs loop state. I suspect it will still be useful to put as much as possible from the continuing {...} block into the condition at the end (esp. if GLSL and HLSL kept expr, expr expressions from C).


Come to think of it, are loops even allowed in continuing {...}? I would expect not, unless infinite loops are UB, because continuing {...} (or at least SPIR-V's concept that WGSL integrated) is required to arrive at the terminator that is the source of the backedge (i.e. the backedge terminator must post-dominate the start of continuing {...}, and unreachable/infinite loops break that. dynamically-potentially-infinite loops might too, but I don't know off the top of my head if they do).

I'm asking because a tree of if-else can be handled as a bunch of nested ?:, if you have expr, expr expressions (since you can use them like Rust block syntax).

Imberflur commented 4 months ago

A switch with just a default case isn't handled well in FXC, see https://github.com/gfx-rs/wgpu/issues/4514

So it would probably need to be written as a do {} while(false). I don't know if this negates the potential improvements to optimization chances? A dummy switch case that is never hit might workaround the bug in FXC, I don't think I have observed any issues in this scenario?