spv-in miscompilation of inlined if statement with return

[khyperia]

This is a follow-up to #939, when I looked at the output of naga's GLSL backend on that code.

The SPIR-V input (and the GLSL that created it) can be found in this shader-playground link. Check the input GLSL program vs. the result GLSL program: if thingy > 5, then I would expect 101010 to be returned. But, 0 is returned unconditionally!

Input GLSL:

#version 460

layout (location = 1) in float thingy;
layout (location = 0) out float fragColor;

float f() {
    if (thingy > 5) {
        return 101010;
    }
    return 0;
}

void main()
{
	fragColor = f();
}

Disassembled SPIR-V, click to expand

; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 10
; Bound: 49
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %thingy %fragColor
               OpExecutionMode %main OriginUpperLeft
               OpSource GLSL 460
               OpName %main "main"
               OpName %thingy "thingy"
               OpName %fragColor "fragColor"
               OpDecorate %thingy Location 1
               OpDecorate %fragColor Location 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
%_ptr_Input_float = OpTypePointer Input %float
     %thingy = OpVariable %_ptr_Input_float Input
    %float_5 = OpConstant %float 5
       %bool = OpTypeBool
%float_101010 = OpConstant %float 101010
    %float_0 = OpConstant %float 0
%_ptr_Output_float = OpTypePointer Output %float
  %fragColor = OpVariable %_ptr_Output_float Output
       %uint = OpTypeInt 32 0
     %uint_0 = OpConstant %uint 0
       %main = OpFunction %void None %3
          %5 = OpLabel
               OpSelectionMerge %46 None
               OpSwitch %uint_0 %41
         %41 = OpLabel
         %42 = OpLoad %float %thingy
         %43 = OpFOrdGreaterThan %bool %42 %float_5
               OpSelectionMerge %45 None
               OpBranchConditional %43 %44 %45
         %44 = OpLabel
               OpBranch %46
         %45 = OpLabel
               OpBranch %46
         %46 = OpLabel
         %48 = OpPhi %float %float_101010 %44 %float_0 %45
               OpStore %fragColor %48
               OpReturn
               OpFunctionEnd

Output GLSL, generated using cargo run --features spv-in,glsl-out -- naga-issue.spv main.frag:

#version 320 es

precision highp float;

float thingy = 0;
float fragColor = 0;
smooth in float _vs2fs_location1;
layout(location = 0) out float _fs2p_location0;

void main1() {
    float phi_48_;
    switch(int(0u)) {
    }
    if((thingy > 5.0)) {
        phi_48_ = 101010.0;
    }
    phi_48_ = 0.0;
    fragColor = phi_48_;
    return;
}

void main() {
    float thingy1 = _vs2fs_location1;
    thingy = thingy1;
    main1();
    _fs2p_location0 = fragColor;
    return;
}

MSL output looks very similar, hence me thinking it's a problem in the SPIR-V frontend, rather than the GLSL backend:

cargo run --features spv-in,msl-out -- naga-issue.spv main.metal

#include <metal_stdlib>
#include <simd/simd.h>


void main1(
    thread float const& thingy,
    thread float& fragColor
) {
    float phi_48_;
    switch(as_type<int>(0u)) {
        default: {
        }
    }
    if (thingy > 5.0) {
        phi_48_ = 101010.0;
    }
    phi_48_ = 0.0;
    fragColor = phi_48_;
    return;
}

struct main2Input {
    float thingy1 [[user(loc1), center_perspective]];
};
struct main2Output {
    float member [[color(0)]];
};
fragment main2Output main2(
  main2Input varyings [[stage_in]]
) {
    float thingy = {};
    float fragColor = {};
    const auto thingy1 = varyings.thingy1;
    thingy = thingy1;
    main1(thingy, fragColor);
    return main2Output { fragColor };
}

[khyperia]

I also just noticed that the layout (location = n) attribute is dropped on the input. I modified the example to use layout (location = 5) for both input and output, and running the GLSL backend in ES profile drops the attribute on the input, and the glsl core profile drops both the input and the output location attributes.

That seems bad, should I file another issue for that?

[kvark]

Thank you for filing this!
At first, I thought this is caused by my hacks in #844.
But actually removing the hacks still shows the problem. Here is the IR generated by SPV-in without hacks:

Emit(
                    [11..11],
                ),
                Switch {
                    selector: [11],
                    cases: [],
                    default: [
                        Emit(
                            [12..13],
                        ),
                        If {
                            condition: [13],
                            accept: [
                                Store {
                                    pointer: [14],
                                    value: [10],
                                },
                            ],
                            reject: [],
                        },
                        Store {
                            pointer: [14],
                            value: [8],
                        },
                    ],
                },
                Emit(
                    [15..15],
                ),
                Store {
                    pointer: [1],
                    value: [15],
                },
                Return {
                    value: None,
                },

You can see that the [14] pointer is still unconditionally assigned after the If.

@MatusT looks like something is not right with the CFG again here. Would you be able to peek at this for the initial diagnostics?

[MatusT]

@MatusT looks like something is not right with the CFG again here. Would you be able to peek at this for the initial diagnostics?

I'll take a look on friday.

[khyperia]

I don't believe this has been fixed. Naga now panics with:

Function [1] 'main' is invalid:
        The `break`/`continue` is used outside of a loop context
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', bin/naga.rs:263:75

Admittedly, it's no longer a miscompilation, but the compilation still fails. Let me know if you'd like me to open a new issue for this new error.

[jimblandy]

This is still broken. SPIR-V file attached (gzipped to make GitHub let me attach it).
naga-940.spv.gz

[MatusT]

It is not yet fixed entirely. There are still 2 issues remaining:

1. Naga's validation doesn't respect break in switch cases (only loops)
2. We discovered that the resolution(it was a hack for few spcific cases) of discrepancy between domination rules in SPIR-V vs scopes of variables was not correct

I don't know when I can find time to fix any of those. Currently, It's only me who is working on CFG code and occasionally @kvark. My contributions are a total side project for me, I am into computer graphics, not compilers.

You will have to either wait until I find time to fix all the issues or somebody else has to do it.

[kvark]

@khyperia I take this is a blocker for you? We understand the issue is a serious one.

The tentative plan was to augment the patch_statement routine in this way:

• if we see an expression used after its block, we'd add a new local variable ("promote")
• emit the Store where it's assigned
• emit one Load inside the block it's assigned, and another after this block
• replace the expression mention with one of these Load

[khyperia]

@khyperia I take this is a blocker for you? We understand the issue is a serious one.

Sorry, I should have been more clear, this isn't directly related to my professional rust-gpu work - well, it is in the sense that someone could write this code, use wgpu, and so be broken, but nobody has yet. I discovered this just through my own personal projects rather than rust-gpu, and from a user perspective rather than compiler developer, there are workarounds I can do in my own personal stuff. The biggest importance is probably just a desire for naga to be a robust shader translation tool (e.g. spirv-cross correctly handles this) to not block potential future adoption by us in our own render engine.

[kvark]

@MatusT something is missing here. I resolved the validation issue with #993, but the generated IR is looking wrong, even without any patching:

                                                If {
                                                    condition: [724],
                                                    accept: [],
                                                    reject: [
                                                        Break,
                                                    ],
                                                },
                                                Break,
                                                Store {
                                                    pointer: [725],
                                                    value: [67],
                                                },

This is taken from
108.zip

What we see here is the Store that comes after Break. It makes no sense.

[jimblandy]

This seems to have been fixed by something, perhaps the new GLSL front end. The following input:

#version 460

layout (location = 1) in float thingy;
layout (location = 0) out float fragColor;

float f() {
    if (thingy > 5.) {
        return 101010.;
    }
    return 0.;
}

void main()
{
	fragColor = f();
}

produces the following SPIR-V for f (full SPIR-V module disassembly given below):

         %15 = OpFunction %float None %16
         %14 = OpLabel
               OpBranch %17
         %17 = OpLabel
         %18 = OpLoad %float %11
         %20 = OpFOrdGreaterThan %bool %18 %float_5
               OpSelectionMerge %21 None
               OpBranchConditional %20 %22 %21
         %22 = OpLabel
               OpBranch %23
         %23 = OpLabel
               OpReturnValue %float_101010
         %24 = OpLabel
               OpBranch %21
         %21 = OpLabel
               OpReturnValue %float_0
               OpFunctionEnd
         %26 = OpFunction %void None %27
         %25 = OpLabel
               OpBranch %28
         %28 = OpLabel
         %29 = OpFunctionCall %float %15
               OpStore %13 %29
               OpReturn
               OpFunctionEnd

This seems to reach the OpReturnValue %float_101010 when the input is greater than 5, and otherwise return zero, as required. The generated WGSL seems okay, too:

fn f() -> f32 {
    let _e2: f32 = thingy1;
    if ((_e2 > 5.0)) {
        {
            return 101010.0;
        }
    }
    return 0.0;
}

; SPIR-V
; Version: 1.0
; Generator: Khronos; 28
; Bound: 42
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %36 "main" %31 %34
               OpExecutionMode %36 OriginUpperLeft
               OpMemberDecorate %_struct_10 0 Offset 0
               OpDecorate %31 Location 1
               OpDecorate %34 Location 0
       %void = OpTypeVoid
        %int = OpTypeInt 32 1
      %int_1 = OpConstant %int 1
      %int_0 = OpConstant %int 0
      %float = OpTypeFloat 32
    %float_5 = OpConstant %float 5
%float_101010 = OpConstant %float 101010
    %float_0 = OpConstant %float 0
 %_struct_10 = OpTypeStruct %float
%_ptr_Private_float = OpTypePointer Private %float
         %11 = OpVariable %_ptr_Private_float Private
         %13 = OpVariable %_ptr_Private_float Private
         %16 = OpTypeFunction %float
       %bool = OpTypeBool
         %27 = OpTypeFunction %void
%_ptr_Input_float = OpTypePointer Input %float
         %31 = OpVariable %_ptr_Input_float Input
%_ptr_Output_float = OpTypePointer Output %float
         %34 = OpVariable %_ptr_Output_float Output
         %15 = OpFunction %float None %16
         %14 = OpLabel
               OpBranch %17
         %17 = OpLabel
         %18 = OpLoad %float %11
         %20 = OpFOrdGreaterThan %bool %18 %float_5
               OpSelectionMerge %21 None
               OpBranchConditional %20 %22 %21
         %22 = OpLabel
               OpBranch %23
         %23 = OpLabel
               OpReturnValue %float_101010
         %24 = OpLabel
               OpBranch %21
         %21 = OpLabel
               OpReturnValue %float_0
               OpFunctionEnd
         %26 = OpFunction %void None %27
         %25 = OpLabel
               OpBranch %28
         %28 = OpLabel
         %29 = OpFunctionCall %float %15
               OpStore %13 %29
               OpReturn
               OpFunctionEnd
         %36 = OpFunction %void None %27
         %30 = OpLabel
         %33 = OpLoad %float %31
               OpBranch %37
         %37 = OpLabel
               OpStore %11 %33
         %38 = OpFunctionCall %void %26
         %39 = OpLoad %float %13
         %40 = OpCompositeConstruct %_struct_10 %39
         %41 = OpCompositeExtract %float %40 0
               OpStore %34 %41
               OpReturn
               OpFunctionEnd

[jimblandy]

Oops, sorry for the noise. I wasn't using the right input.

[jimblandy]

Okay, the interesting SPIR-V input is this:

       %main = OpFunction %void None %3
          %5 = OpLabel
               OpSelectionMerge %46 None
               OpSwitch %uint_0 %41

         %41 = OpLabel
         %42 = OpLoad %float %thingy
         %43 = OpFOrdGreaterThan %bool %42 %float_5
               OpSelectionMerge %45 None
               OpBranchConditional %43 %44 %45

         %44 = OpLabel
               OpBranch %46

         %45 = OpLabel
               OpBranch %46

         %46 = OpLabel
         %48 = OpPhi %float %float_101010 %44 %float_0 %45
               OpStore %fragColor %48
               OpReturn
               OpFunctionEnd

Roughly, this is:

switch (0) {
  default:
    if (thingy > 5) {
        phi = 101010;
        break;
    }
    phi = 0;
}
return phi;

The Naga IR generated for this has completely failed to pack up the OpSwitch instruction's cases:

        body: [
            Emit((
                start: 10,
                end: 11,
            )),
            Switch(
                selector: 11,
                cases: [],
                default: [],
            ),
            Emit((
                start: 11,
                end: 13,
            )),
            If(
                condition: 13,
                accept: [],
                reject: [
                    Break,
                ],
            ),
            Store(
                pointer: 14,
                value: 9,
            ),
            Break,
            Store(
                pointer: 14,
                value: 10,
            ),
            Emit((
                start: 14,
                end: 15,
            )),
            Store(
                pointer: 1,
                value: 15,
            ),
            Return(
                value: None,
            ),
        ],

That If should be contained in the default child of the Switch statement.

[jimblandy]

Oh, apparently the empty Switch default list doesn't mean anything. The SPIR-V front end has a special case that transforms

switch (x) {
  default:
    ...
}

into

switch (x) {
  default:
}
...

[jimblandy]

Aaaaand the issue is that that transformation is invalid if the ... contains Break statements.

[kvark]

As discussed on Matrix, the plan for this specific issue is to handle the degenerate conditions a bit better.
When lifting the statements out of their blocks, (like the only "default" case in a "switch"), we'd stop whenever we see Break or any construct involving Break. That should include all of the emitted expressions that need to be visible after that degenerate condition. If there is another expression somewhere down the block, it's no longer dominating continuation of the block, so it can't be used there anyway.

[jimblandy]

I'm not sure the approach of simply eliminating degenerate control flow can work.

To sum things up:

The whole reason we're playing these games at all is that Naga IR and SPIR-V have different rules for where expressions / instructions can be used. Whereas Naga IR scopes expressions according to the statement tree, SPIR-V lets you use an instruction's value as long as the instruction dominates the use - and then further gives you phi nodes that can use its value even in code it doesn't dominate.

Since SPIR-V is more forgiving than Naga IR, translation from the former to the latter entails introducing temporary variables to hold values that would otherwise be out of scope. Our SPIR-V front end depends on phi nodes to tell us when these variables need to be introduced: a value must be stashed in an introduced temporary variable exactly if it is the input to a phi.

But this approach fails in the presence of "degenerate control flow": any SPIR-V control flow like a loop, selection, or switch that appears to be conditional, but which actually contains blocks that do dominate the following code. The example at hand is:

switch (1) {
  default:
    ... /*A*/
}
... /*B*/

In this case, despite being part of a "conditional" structure (the switch), block A dominates block B. SPIR-V thus permits B to use values from A without a phi, even though Naga IR would require saving values of a Switch statement's branches in variables before using them in subsequent code.

Another example of degenerate control flow might be the SPIR-V equivalent of:

loop {
    if (cond) {
        ... /*A*/
    } else {
        ... /*B*/
        break;
    }
    ... /*C*/
}

In this case, block A dominates block C, even though A is within an if statement. Again, SPIR-V permits C to use values from A without a phi, even though Naga IR requires temporaries.

Each of these cases can be corrected to remove the degenerate control flow. The
first is obviously equivalant to:

... /*A*/
... /*B*/

and the second is equivalent to:

loop {
  if (cond) {
  } else {
      break;
  }
  ... /*A*/
  ... /*B*/
}

But at this point it's getting hard to feel confident that we've covered all cases.

[jimblandy]

Another case to consider:

switch (a) {
  default:
    if (b) {
        if (c) {
            break;
        } else {
            .../*D*/
        }
    } else {
        if (d) {
            ...
            break;
        } else {
            ...
            break;
        }
    }
    .../*E*/
}

In this case, D dominates E, only because every other path in the if tangle breaks. There will be no phis in E to help us out. This can be converted to:

switch (a) {
  default:
    if (b) {
        if (c) {
            break;
        }
    } else {
        if (d) {
            ...
            break;
        } else {
            ...
            break;
        }
    }
    .../*D*/
    .../*E*/
}

If E is live at all, anything in the if tangle that dominates it must be safe to move out of the if tangle entirely.

[kvark]

@jimblandy thank you for an excellent description of what we are trying to solve. Based on your examples, I'm not seeing why the proposed patching strategy wouldn't work. It would be really nice if we do all this without uplifting any expressions to variables, just from the "perfectionist" position here :)
The complexity of the cases you looked it, I think, would just be addressed by ensuring that the degenerate condition patching goes in the depth-first manner. I.e. the cases are complex when these degenerate conditions are nested into each other, and the patching should address the innermost first, then the outer ones become straightforward.

[jimblandy]

How would we handle this one?

switch (0) {
  default:
    if (x) {
        ... /*A*/
        if (y) {
            ... /*B*/
            if (z) {
                ... /*C*/
            } else {
                break;
            }
        } else {
            break;
        }
    } else {
        break;
    }
    ... /*D*/
}

In this case A dominates B dominates C dominates D, so there will be no
phis. But for Naga to avoid introducing temporaries, this has to be flattened
out. I guess it would have to become:

switch (0) {
  default:
    if (!x) {
        break;
    }
    ... /*A*/
    if (!y) {
        break;
    }
    ... /*B*/
    if (!z) {
        break;
    }
    ... /*C*/
    ... /*D*/
}

[jimblandy]

Here's another degenerate case:

loop {
    A;
    break;
}
B;

[jimblandy]

I think what this shows is that, as we walk a degenerate switch's default block, we need to ask both:

• Can anything here possibly dominate the code after the switch? For example:

  switch (0) {
    default:
      A;
  }
  B; // dominated by A
  

• Can anything here possibly dominate the rest of the default block? For example:

  switch (0) {
    default:
      if (x) {
          A;
      } else {
          break;
      }
      B; // dominated by A
  }
  

Both situations could introduce phi-less references to expressions from nested statements.

[jimblandy]

Here's another case we don't handle, for similar reasons, that has nothing to do with switch statements. We get:

Function [1] '' is invalid: 
	Expression [10] is invalid
	Used by a statement before it was introduced into the scope by any of the dominating blocks

This SPIR-V is basically:

fn f(left: i32, right: i32, which: bool) {
    if which {
        return left + 1;
    } else {
        temp = right + 1; // no var, just a SPIR-V insn
    }
    return temp; // refers directly to add insns
}

Because of the return, the else dominates the rest of the function.

Here's the SPIR-V:

                OpCapability Shader
                OpCapability Linkage
                OpMemoryModel Logical GLSL450

         %int = OpTypeInt 32 1
        %bool = OpTypeBool
 %either_type = OpTypeFunction %int %int %int %bool
     %const_1 = OpConstant %int 1

      %either = OpFunction %int None %either_type
        %left = OpFunctionParameter %int
       %right = OpFunctionParameter %int
       %which = OpFunctionParameter %bool
    %ob_label = OpLabel
                OpSelectionMerge %merge None
                OpBranchConditional %which %take_left %take_right

   %take_left = OpLabel
   %left_plus = OpIAdd %int %left %const_1
                OpReturnValue %left_plus

  %take_right = OpLabel
  %right_plus = OpIAdd %int %right %const_1
                OpBranch %merge

       %merge = OpLabel
                OpReturnValue %right_plus
                OpFunctionEnd

The IR is:

            body: [
                If {
                    condition: [8],
                    accept: [
                        Emit(
                            [9..9],
                        ),
                        Return {
                            value: Some(
                                [9],
                            ),
                        },
                    ],
                    reject: [
                        Emit(
                            [10..10],
                        ),
                    ],
                },
                Return {
                    value: Some(
                        [10],
                    ),
                },
            ],

[jimblandy]

Here's how I think this "concealed dominator" transformation has to work.

Since continue and return statements can introduce concealed dominators just as well, we must generalize this to address all of these cases. But for the moment, I want to just focus on degenerate switches: I don't see the whole solution yet, so I want to follow Feynman's advice here:

If you can't solve a problem, consider a simpler version of the problem that you do know how to solve. But don't just say you can solve it: actually solve it.

First of all, as we consider the default of a degenerate switch, there are two possible regions that could have concealed dominators:

switch (c) {
  default:
    A;  // concealed dominator of D
    if (x) {
        break;
    } else {
        B; // concealed dominator of C
    }
    C; // "tail", dominated by A, B
}
D; // "outside", dominated by A

So we need to watch out for both the code after the switch, and the tail of the default block.

We can inspect each statement in the default block to determine if it never breaks out of the switch, always breaks, or sometimes breaks. This is decidable because we don't care about data at all, only control flow: SPIR-V's SSA only cares about the shape of the graph, so OpConditionalBranch %true ... is still a conditional branch.

Starting at the head of the default block, all statements that never break dominate both the tail and outside. Since outside will use them without phis, they must be moved before the switch:

A; // assume never breaks
switch {
    ...
}
D;

After the first statement that sometimes or always breaks, nothing in the default block dominates outside, so we don't need to think about moving that out. We only need to worry about the tail.

Nested loops and switches capture any break statements they contain, so they're in the "never break" category. We only need to worry about if statements.

So suppose we have:

if (a) { B; } else { C; }

Each of B and C never, sometimes, or always breaks. If B definitely
breaks, then nothing in it can dominate the following tail, but everything in C does.
We can transform this into:

if (a) { B; }
C;

And symmetrically, if C definitely breaks, we can turn it into this:

if (!a) { C; }
B;

Then, processing of the tail must continue starting with the code moved out of the if, since those may contain if branches of their own.

Otherwise, B and C either sometimes or never break. But in any case, neither dominate the rest of the tail, which must use phis to refer to its values, so the if can be left alone.

An else-less if is just C being empty, so it can be handled like an if whose 'else' never breaks.

Suppose we apply this algorithm to a variant of the tangle I showed earlier. Assume the letter statements never break.

switch (0) {
  default:
    Q;
    if (x) {
        A;
        if (y) {
            B;
            if (z) {
                break;
            } else {
                C;
            }
        } else {
            break;
        }
    } else {
        break;
    }
    D;
}
E;

Since Q never breaks, but dominates E, it must be moved before the switch.

Since if (x) sometimes breaks, we're done worrying about dominators of E.

In if (x), the 'then' branch sometimes breaks, and the 'else' clause always breaks. So we negate the condition and move the 'then' branch out of the if. Here's the score:

Q;
switch (0) {
  default:
    if (!x)
        break;

    A;
    if (y) {
        B;
        if (z) {
            break;
        } else {
            C;
        }
    } else {
        break;
    }
    D;
}
E;

A never breaks. if (y) sometimes breaks. Its 'else' clause always breaks, so we negate the condition and move the 'then' clause out of the if:

Q;
switch (0) {
  default:
    if (!x)
        break;
    A;
    if (!y)
        break;
    B;
    if (z) {
        break;
    } else {
        C;
    }
    D;
}
E;

B never breaks. if (z) sometimes breaks. The 'then' clause always breaks, so we move the 'else' clause out of the if, leaving the condition unchanged:

Q;
switch (0) {
  default:
    if (!x)
        break;
    A;
    if (!y)
        break;
    B;
    if (z) {
        break;
    }
    C;
    D;
}
E;

This looks like a right answer for that input: all the dominators that need to stay in the switch are lined up, and Q is hoisted out of the switch altogether.

I haven't thought about how to implement this. The 'never/sometimes/always breaks' analysis needs to be computed once and retained for statements even when they're moved, because otherwise we'll have to re-analyze subtrees each time we encounter them, and the algorithm will be quadratic.

[jimblandy]

This kind of analysis is really different from what I've usually heard people talking about. We're kind of being led around by the nose by IR restrictions. But I'm pursuing it because the alternative seems to be to make deep changes to Naga's IR, which would be disruptive.

[jimblandy]

This doesn't make the original SPIR-V input work, unfortunately, since it seems like there's a spurious Break in the function before the reveal_dominators algorithm ever gets to see it:

[src/front/spv/reveal_doms.rs:15] &default = [
    Emit(
        [12..13],
    ),
    If {
        condition: [13],
        accept: [
            Store {
                pointer: [14],
                value: [9],
            },
            Break,
        ],
        reject: [
            Break,
        ],
    },
    Store {
        pointer: [14],
        value: [10],
    },
]

The Break in the reject branch shouldn't be there.

[jimblandy]

Another lovely case:

loop {
    if (a) {
        A;
        if (b) {
            B;
            if (c) {
                C;
                break;
            }
        }
    }
    D;
}
E;

Here, A, B, and C dominate E. Only C can be moved out of the loop. [fixed]

[jimblandy]

spirv-val accepts the SPIR-V version of the above.
loops.spv.gz

[jimblandy]

More simply, I think this can't be transformed into Naga IR without introducing temporary variables, even though it requires no phis in SPIR-V:

loop {
    A;
    if (b)
        break;
    C;
}
D;

A dominates D, so D can use A's values without phis. But A must be inside the loop, so it cannot be moved in Naga IR to put its expressions in scope for D.

[jimblandy]

Together with #1304, #1294 seems to fix this.

[jimblandy]

We now generate the following SPIR-V for the original input file:

var<private> thingy1: f32;
var<private> fragColor: f32;

fn main1() {
    var phi_48_: f32;

    switch(bitcast<i32>(0u)) {
        default: {
            let _e11: f32 = thingy1;
            if ((_e11 > 5.0)) {
                phi_48_ = 101010.0;
                break;
            }
            phi_48_ = 0.0;
            break;
        }
    }
    let _e14: f32 = phi_48_;
    fragColor = _e14;
    return;
}

[[stage(fragment)]]
fn main([[location(1)]] thingy: f32) -> [[location(0)]] f32 {
    thingy1 = thingy;
    main1();
    let _e3: f32 = fragColor;
    return _e3;
}

Excessive temporaries aside, this looks correct to me.

If there's still a problem, feel free to re-open.