Skip to content

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Sep 11, 2025

SPIR-V allows what it calls "merge edges" (edges from a structured header block, to its merge block), e.g.:

if cond {
    asm!("OpKill");
}
// ...

can directly use the merge block as the "else" edge of the conditional branch:

  OpSelectionMerge %merge None
  OpBranchConditional %cond %then %merge

%then = OpLabel
  OpKill

%merge = OpLabel
  ; ...

but before this PR, SPIR-T -> SPIR-V lifting was not taking advantage of that, instead emitting:

  OpSelectionMerge %merge None
  OpBranchConditional %cond %then %else

%then = OpLabel
  OpKill

%else = OpLabel
  OpBranch %merge

%merge = OpLabel
  ; ...

This pattern became most obvious on Rust-GPU shaders with a lot of panics from e.g. bounds-checking (which get turned into conditional "OpReturn from entry point"), but not from inspecting the SPIR-V output.

Instead, I ended up noticing that spirv-opt has a merge-blocks pass, which can remove the slight inefficiency from situations like the above example, but it's very slow for what it does (and it was the slowest pass in spirv-opt --time-report, for the SPIR-V module I was looking at).
(if linear, that would mean around 200µs per trivial basic block, which feels absurd given how simple the check in this PR is, so I suspect it's accidentally quadratic or worse, but I haven't looked at the C++ code to confirm either)

EDIT: with this PR used in Rust-GPU, the specific testcase which prompted it (schell/renderling@d9f4d6f) has the time spent in spirv-opt shrink by ~20% (4s -> 3.21s), which is even better than the 0.5s I was expecting.

@eddyb eddyb requested a review from a team September 11, 2025 16:00
@eddyb eddyb enabled auto-merge September 11, 2025 16:25
Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have tests? This seems super testable.

.then(|| targets[0])
};
if let Some(target_of_target) = target_is_trivial_branch {
// FIXME(eddyb) what does it mean for this to not be true?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put an error or panic in here so if we hit it we are aware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants