Skip to content

Commit

Permalink
spv/lift: use loop body entry (instead of exit), as the "continue" ta…
Browse files Browse the repository at this point in the history
…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>
  • Loading branch information
eddyb committed Dec 9, 2022
2 parents b050a10 + 44614bf commit cda161b
Showing 1 changed file with 18 additions and 7 deletions.
25 changes: 18 additions & 7 deletions src/spv/lift.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,14 @@ enum Merge<L> {
/// The label just after the whole loop, i.e. the `break` target.
loop_merge: L,

/// The label just after the loop body, i.e. the `continue` target.
body_merge: L,
/// A label that the back-edge block post-dominates, i.e. some point in
/// the loop body where looping around is inevitable (modulo `break`ing
/// out of the loop through a `do`-`while`-style conditional back-edge).
///
/// SPIR-V calls this "the `continue` target", but unlike other aspects
/// of SPIR-V "structured control-flow", there can be multiple valid
/// choices (any that fit the post-dominator/"inevitability" definition).
loop_continue: L,
},
}

Expand Down Expand Up @@ -724,7 +730,12 @@ impl<'a> FuncLifting<'a> {
target_phi_values: FxIndexMap::default(),
merge: Some(Merge::Loop {
loop_merge: CfgPoint::ControlNodeExit(control_node),
body_merge: CfgPoint::RegionExit(*body),
// NOTE(eddyb) this may seem weird, but see the
// note on `Merge::Loop`'s `loop_continue`
// field - in particular, for SPIR-T loops, we
// can pick any point before/after/between
// `body`'s `children` and it should be valid.
loop_continue: CfgPoint::RegionEntry(*body),
}),
},
}
Expand Down Expand Up @@ -863,7 +874,7 @@ impl<'a> FuncLifting<'a> {
Merge::Selection(a) => (a, None),
Merge::Loop {
loop_merge: a,
body_merge: b,
loop_continue: b,
} => (a, Some(b)),
};
[a].into_iter().chain(b)
Expand Down Expand Up @@ -1323,7 +1334,7 @@ impl LazyInst<'_, '_> {
},
Self::Merge(Merge::Loop {
loop_merge: merge_label_id,
body_merge: continue_label_id,
loop_continue: continue_label_id,
}) => spv::InstWithIds {
without_ids: spv::Inst {
opcode: wk.OpLoopMerge,
Expand Down Expand Up @@ -1524,10 +1535,10 @@ impl Module {
}
Merge::Loop {
loop_merge,
body_merge,
loop_continue,
} => Merge::Loop {
loop_merge: func_lifting.label_ids[&loop_merge],
body_merge: func_lifting.label_ids[&body_merge],
loop_continue: func_lifting.label_ids[&loop_continue],
},
})
}))
Expand Down

0 comments on commit cda161b

Please sign in to comment.