[spv-in] Conditional loop backedge not lowered to Statement::Loop with break_if expression
#1977
Comments
teoxoy
added
kind: bug
|
I finally had some time to analyze this issue and it seems to be a combination of smaller issues. The part that's is causing all of this is %15 = OpLabel ; validCFG/StructurallyReachableBlock$2
// ...
OpBranchConditional %temp_15_6 %13 %11To note here is the conditional branch that goes to either The second target is tricky and the problem is further discussed in this issue gfx-rs/wgpu#4388 The first target is a break and should be handled as such, but naga's validator seems to disallow |
|
The grammar isn't especially clear, but Further statements after the This is supposed to exactly match SPIR-V's requirements for continuing blocks:
Those rules forbid breaks in continue constructs, except that the final backward branch may be conditional. |
|
It doesn't look like Naga has implemented that final |
|
By using the Naga CLI, I was able to reduce one of my own errors to: (click to open - it's not that relevant, see later below)fn f(cond: bool) {
loop {
// ... loop body ...
continue;
continuing {
if cond {
} else {
break;
}
}
}
}$ naga minimal.wgsl
error:
┌─ minimal.wgsl:1:1
│
1 │ ╭ fn f(cond: bool) {
2 │ │ loop {
3 │ │ // ... loop body ...
4 │ │ continue;
· │
8 │ │ break;
│ │ ^^^^^ invalid break
9 │ │ }
10 │ │ }
11 │ │ }
│ ╰─────^ naga::Function [1]
Function [1] 'f' is invalid:
The `break` is used outside of a `loop` or `switch` contextIs this the same issue, @jimblandy? I wonder if the title of this issue should be changed to remove "switch", in that case. Though, reducing the WGSL is a bit misleading, since what I'd really want to do is reduce the $ naga --validate 0 minimal.wgsl minimal.spv
$ spirv-val minimal.spv
error: line 24: The continue construct with the continue target '12[%12]' is not structurally post dominated by the back-edge block '13[%13]'
%13 = OpLabelBut by starting from OpCapability Shader
OpCapability Linkage
OpMemoryModel Logical GLSL450
%void = OpTypeVoid
%bool = OpTypeBool
%typeof_f = OpTypeFunction %void %bool
%f = OpFunction %void None %typeof_f
%cond = OpFunctionParameter %bool
%entry = OpLabel
OpBranch %loop
%loop = OpLabel
OpLoopMerge %loop_exit_merge %loop_continue None
OpBranch %loop_body
%loop_body = OpLabel
OpBranch %loop_continue
%loop_continue = OpLabel
; OpSelectionMerge %loop_back_edge None
; OpBranchConditional %cond %loop_break %loop_back_edge
OpBranchConditional %cond %loop_exit_merge %loop
; %loop_break = OpLabel
; OpBranch %loop_exit_merge
; %loop_back_edge = OpLabel
; OpBranch %loop
%loop_exit_merge = OpLabel
OpReturn
OpFunctionEnd(the commented out instructions are the original ones, that I had to look at the original larger shader to know what to replace them with) $ spirv-as minimal.spvasm -o minimal.spv
$ spirv-val minimal.spv
$ naga minimal.spv
Function [1] '' is invalid:
The `break` is used outside of a `loop` or `switch` context@jimblandy so it feels like "SPIR-V frontend: conditional backedge not lowered to |
|
As suspected, this breaks idiomatic (i.e. using conditional backedges) SPIR-V $ cat do_while.vert
#version 450
void f(bool cond) {
do {} while(cond);
}
void main() {
f(true);
}
$ glslangValidator -V do_while.vert -o do_while.spv
do_while.vert
$ spirv-val do_while.spv
$ naga do_while.spv
Function [1] 'f(b1;' is invalid:
The `break` is used outside of a `loop` or `switch` contextBy comparison, Naga's own GLSL frontend produces effectively this (w/o any fn f(cond: bool) {
loop {
if !cond {
break;
}
}
return;
} |
…rget. (#10) As we have no `break` edges out of SPIR-T loop bodies, any point in the loop body (not nested in another selection/loop) is a valid "continue" target, and we can spare a block by making *the whole body* that. In [WGSL loops](https://gpuweb.github.io/gpuweb/wgsl/#loop-statement), this corresponds to moving the rest of the loop body (*not* using `break`/`continue`) into the `continuing {...}` block. <sub>(but examples are hard because `break if` isn't implemented yet in Naga, and our `do`-`while`-style loops use SPIR-V "conditional back-edges" which in WGSL *require* `break if` - see gfx-rs/naga#1977 (comment) for more details/context)</sub> For `tests/data/for-loop.wgsl.spvasm` (after `spirv-as` -> `spv-lower-link-lift` -> `spirv-dis`), the change is: ```diff @@ -25,5 +25,5 @@ %22 = OpLabel - %13 = OpPhi %int %int_1 %21 %16 %27 - %14 = OpPhi %int %int_1 %21 %17 %27 - OpLoopMerge %28 %27 None + %13 = OpPhi %int %int_1 %21 %16 %26 + %14 = OpPhi %int %int_1 %21 %17 %26 + OpLoopMerge %28 %23 None OpBranch %23 @@ -44,4 +44,2 @@ %17 = OpPhi %int %20 %24 %11 %25 - OpBranch %27 - %27 = OpLabel OpBranchConditional %15 %22 %28 ``` <sub>(on the "after" side I had to use `sed 's/%27/%28/g'` on to make the diff more readable wrt renumbering)</sub>
jimblandy
changed the title
Statement::Loop with break_if expression
Naga (ab2806e) rejects this SPIR-V:
We get:
The 'break' is used outside of a 'loop' or 'switch' contextThe SPIR-V is valid, and spirv-val accepts it. There is no "break" outside any switch.
This is exactly the same as this issue.
The text was updated successfully, but these errors were encountered: