the multiple jumps to %48 seem suspect to me, since that is not the merge block.

I'm guessing they're loop breaks, because of OpLoopMerge %48 %43 None?
Given "a break block is valid only for the innermost loop it is nested inside of" in the spec, but no limitation around the loop break being nested in a switch (though the other way is limited), I think this is legal?

Thank you for filing!
Summoning our SPIR-V expects @JCapucho and @jimblandy

Just FYI, these confusing

OpSelectionMerge %79 None
OpSwitch %uint_0 %8 0

are added by spirv-opt's --merge-return pass (used with -O):

https://github.com/KhronosGroup/SPIRV-Tools/blob/1082de6bb32fc425623a6a218c5295bfd4db9a09/source/opt/merge_return_pass.h#L45-L57

I'm encountering this problem with rust-gpu shader that peforms an OpKill.

As a temporary workaround, I've written an optimisation pass that removed the redundant branch: https://github.com/expenses/spirv-extra-opt-passes/blob/194f01af2957eab95195021f8e86e1cf5a7f0884/src/lib.rs#L841-L902. It's a bit hacked together and probably won't work outside of very simple cases because I don't correct the new merge block in subsequent OpSelectionMerges. I'm not 100% sure this is possible though, the --merge-return pass might exist for a reason :P

That SPIR-V validates fine, so we can assume the breaks follow the structured control flow rules, and our front end ought to be handling it. (Naga does not fully validate SPIR-V; that's too big a job.)

The OpSwitch has three cases: %66, %67, and %68, which is both the merge block and the 1 case. %66 and %67 end with branches to %48, which is the containing loop's merge block, whereas %68 branches to the loop's continue block, so it goes around the loop again.

The SPIR-V front end records all merge blocks, so it recognizes that the OpBranch instructions at the ends of %66 and %67 are branching to the loop merge block, and calls merger to add the appropriate ending to body.

But I think the problem is here, where we try to add the right BodyFragment to the end of the Body that represents the switch case:

MergeBlockInformation::LoopMerge | MergeBlockInformation::SwitchMerge => {
    BodyFragment::Break
}

(See the docs on Body and BodyFragment for background.)

A BodyFragment::Break doesn't carry enough information to distinguish between a break from the enclosing switch and a break out of the nearest enclosing loop. SPIR-V allows both (as the structured control flow rules explain), whereas Naga IR only allows a break out of the innermost enclosing loop or switch.

Changing Naga IR to provide "break from innermost loop" would require supporting that IR in all the back ends. I think it would be better for the SPIR-V front end to introduce a bool variable and do a conditional break after the switch.

cc @JCapucho

A BodyFragment::Break doesn't carry enough information to distinguish between a break from the enclosing switch and a break out of the nearest enclosing loop. SPIR-V allows both (as the structured control flow rules explain), whereas Naga IR only allows a break out of the innermost enclosing loop or switch.

Changing Naga IR to provide "break from innermost loop" would require supporting that IR in all the back ends. I think it would be better for the SPIR-V front end to introduce a bool variable and do a conditional break after the switch.

I agree with you in everything, this will probably need us to maintain depth information for continues and breaks (something that we discussed initially but didn't implement because it didn't seem necessary, didn't age well), the hardest part will be adding the Ifs to the block since we need to possibly add it in multiple places.

I'm not planning to work on this issue in the near future but if anyone wants guidance I'm available.

Note that since we last looked at this, the section on "Structured control flow" in the SPIR-V section has been completely rewritten. Double-check me on this, but I think the new language says that, although it is okay to break out of a loop from within enclosed selection and switch constructs, you can't break out of more than one level of loop.

This is equivalent to having two kinds of break: loop breaks, and switch breaks. You don't need labels or depth counts or anything like that, to capture this.

I'm not sure that really simplifies anything. The back ends other than SPIR-V still can't express this, so I think it still makes sense to keep it out of Naga IR, and make the front end responsible for injecting the bools and branches.

This is equivalent to having two kinds of break: loop breaks, and switch breaks.

I agree, but also I would go further and say that conflating the two is a syntactic confusion comparable to conflating break and return etc. - the only common thing between loop break and switch break is that they're both a forward branch, but you can say the same thing about continue (forward branch to the continuing {...} section).

I think it still makes sense to keep it out of Naga IR

I may want to take this on, if I can confirm spirv-opt easily generates this even in the absence of Rust-GPU.

Managed to make a pure Naga+spirv-opt repro:

• loop-break-from-switch.wgsl:

  @fragment
  fn main(@location(0) dyn_case: i32) -> @location(0) i32 {
      loop {
          switch(dyn_case) {
              case 0: {
                  return 0;
              }
              default: {
                  break;
              }
          }
          return -9;
      }
      return -9999;
  }

• naga loop-break-from-switch.{wgsl,spv}
  spirv-opt -O loop-break-from-switch.spv -o loop-break-from-switch.opt.spv
  naga loop-break-from-switch.opt.{spv,wgsl}

• loop-break-from-switch.opt.wgsl:

  var<private> global: i32;
  var<private> global_1: i32;
  
  fn function() {
      let _e8 = global;
      loop {
          switch _e8 {
              case 0: {
                  global_1 = 0;
                  break;
              }
              default: {
                  break;
              }
          }
          global_1 = -9;
          break;
      }
      return;
  }
  
  @fragment 
  fn main(@location(0) param: i32) -> @location(0) i32 {
      global = param;
      function();
      let _e3 = global_1;
      return _e3;
  }

It's pretty clear that the WGSL post-spirv-opt always returns -9, despite that not being the case originally (and it could be worse, e.g. the loop may become infinite, if the return -9; wasn't there, etc.).