Skip to content

Sema: Fix switch loop OPV cond lowering #24720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Justus2308
Copy link
Contributor

@Justus2308 Justus2308 commented Aug 6, 2025

Resolves #21550
Resolves #24126
Resolves #21772

Special-cases runtime loops with a condition with one possible value and lowers them to a regular loop instead of a loop_switch_br construct. Also covers a switch loop with a single else prong, see comment below.
The implementation is basically a simplified version of analyzeSwitchRuntimeBlock and the code following it in zirSwitchBlock that makes lots of assumptions and omits any value corruption safety checks or prong branch hints.

This patch takes advantage of the symmetry between loop_switch_br-switch_dispatch and loop-repeat and relies on backends being able to handle a loop targeted by multiple repeats. This pattern should be possible but has never been emitted by Sema until now.
The added tests skip the self-hosted spirv backend because it doesn't support this pattern yet.

Also fixes a typo in Liveness.

@Justus2308 Justus2308 force-pushed the loop-switch-opv branch 3 times, most recently from ca67759 to 007b0ee Compare August 6, 2025 23:36
@Justus2308
Copy link
Contributor Author

Justus2308 commented Aug 6, 2025

I realized that this patch also works for the general case of a switch loop with a single non-inlined else/_ prong and no other prongs so I added that. It's a free optimization because we already check for that case anyway for some safety checks in zirSwitchBlock.

@Justus2308 Justus2308 marked this pull request as draft August 7, 2025 01:23
@Justus2308
Copy link
Contributor Author

Justus2308 commented Aug 7, 2025

This is kinda blocked by my own PR (#24381) for now lol, I'll continue working on it once that's merged
I might expand on this to also handle switch (cond) { else => {} } once that's happened, but that will be a separate PR

@Justus2308 Justus2308 marked this pull request as ready for review August 10, 2025 23:48
@alexrp alexrp requested a review from mlugg August 14, 2025 01:20
@Justus2308
Copy link
Contributor Author

Justus2308 commented Aug 16, 2025

Ok I believe I've found a much better solution to this problem now that can reuse most of the code that's already there and handles switch (cond) { else => {} }. It does include some std.meta abuse (nothing too bad though), I'm open to suggestions for nicer alternatives :)

Special-cases runtime switches with a condition with
one possible value or a singular else prong with no
other prongs and lowers them to a regular `block`/`loop`
instead of a `switch_br`/`loop_switch_br`.
@Justus2308
Copy link
Contributor Author

I managed to get rid of the std.meta usage. Sorry for reworking this PR so much, I promise this is the last major change!

@andrewrk
Copy link
Member

Sorry for reworking this PR so much

that's no problem, I for one appreciate when someone takes the time to get it right.

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