Skip to content
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

Computed gotos #19459

Closed
wants to merge 2 commits into from
Closed

Conversation

EzequielRamis
Copy link

@EzequielRamis EzequielRamis commented Mar 28, 2024

Closes #8220.

I took the risk of contributing because it's my first time working with something complex like the zig source code, but I'm currently doing a college project with it and I want to boost the performance with this feature. About the bounty, it's ok if the deadline is missed, as it's personally a nice incentive but secondary.

Below you'd find a list of what I was able to do, what needs to be corrected, what is missing and doubts/things to consider, expecting many items to have phrases like "I have no idea".

TODO

  • Better tests.
  • Backends: I've only touched the llvm one.
  • Fix zig fmt: I think the parsing works, but I couldn't found why this command does not and I don't know how to debug it properly.
  • Fix Sema:
    • I have no idea how to handle prong captures with the dispatched operands, specially if they are references. Maybe a phi instruction must be created in Air?
    • I have no idea how to link the switch_dispatch instruction with its labeled switch_br without having type or liveness resolution errors.
  • Fix codegen: Related to the last bullet, I want to identify a dispatch table by its switch. Adding this Air.SwitchBr field solves it but I don't think it's nice/good enough.
  • Direct jump: For now it only works when it's run at compile time.
  • Indirect jump: I didn't test any created function like airSwitchIndirectBr or indirectBr yet.

Doubts

Copy link
Member

@mlugg mlugg left a comment

Choose a reason for hiding this comment

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

I've not reviewed this properly, but for a first time contribution, this looks like impressive work! I've left a few preliminary comments, but will wait until this is a little more finished before doing a full review.

lib/std/zig/Zir.zig Outdated Show resolved Hide resolved
src/Air.zig Outdated Show resolved Hide resolved
src/Air.zig Outdated Show resolved Hide resolved
test/cases/switch_continue.0.zig Outdated Show resolved Hide resolved
src/codegen/llvm.zig Show resolved Hide resolved
src/Sema.zig Outdated Show resolved Hide resolved
src/Sema.zig Outdated Show resolved Hide resolved
src/Sema.zig Show resolved Hide resolved
src/Sema.zig Show resolved Hide resolved
@wrongnull
Copy link
Contributor

This pull request makes it possible to write code like this

const bit: u1 = 0;

label: switch (bit) {
    0 => continue :label 1,
    1 => continue :label 0,
}

It's obvious that such code results in infinite loop. So far I'm not quite sure if it will possible to compiler to infer this statically. But if so, I'm sure that compiler should produce diagnostics with a note something like this: note: consider using 'while(true)'
Should I make a separate proposal for this?

@silversquirl
Copy link
Contributor

@wrongnull

  1. that looks very difficult to detect in a reliable way
  2. if someone's reaching for this syntax, they probably already know about while loops :)

@EzequielRamis
Copy link
Author

@wrongnull it does catch it if the switch is run at compile time

@wrongnull
Copy link
Contributor

1. that looks very difficult to detect in a reliable way

Are you completely sure about that? At least the compiler is able to detect reaching unreachable code at compile time. As far as I'm concerned there is a control flow graph for thing like that.

2. if someone's reaching for this syntax, they probably already know about while loops :)

good point. Maybe the error message is redundant here. That's why I asked a question about proposal

@mlugg
Copy link
Member

mlugg commented Mar 28, 2024

Having the switch never result in a value explicitly is allowed: in fact, it's probably the main or only way we'll utilize the syntax in the compiler itself, since the termination case would just return.

@mlugg
Copy link
Member

mlugg commented Mar 30, 2024

@EzequielRamis genuinely great work on this PR -- it needs some tinkering, but it's incredibly high-quality for a first-time PR.

Would you like to continue working on this? If you're interested in solving the TODOs and issues yourself, I'm very happy for you to do so, and I will help you where I can. However, if you'd prefer to fast-track the actual feature, or if you're too busy to dedicate much more time to it, I can take over this branch once I'm done with my current branch. Your call!

@EzequielRamis
Copy link
Author

@mlugg thank you for your kind words! It would be great if you could continue with the todos and the other comments I've left, since I need time for my studies. Before that I will push some fixes of the reviews you've written.

@EzequielRamis
Copy link
Author

@mlugg there you have it

@mlugg
Copy link
Member

mlugg commented Mar 30, 2024

Okay, no worries -- I'll pick this up within the next few days. Thanks!

@drfuchs
Copy link

drfuchs commented Apr 29, 2024

It's obvious that such code results in infinite loop. So far I'm not quite sure if it will possible to compiler to infer this statically.

You can detect some infinite loops, but in general it's undecidable; see "The Halting Problem" from the Theory of Computer Science.

@wrongnull
Copy link
Contributor

It's obvious that such code results in infinite loop. So far I'm not quite sure if it will possible to compiler to infer this statically.

You can detect some infinite loops, but in general it's undecidable; see "The Halting Problem" from the Theory of Computer Science.

I understand it well. I told about detecting loops in control flow graph statically when it's possible

@andrewrk
Copy link
Member

andrewrk commented Jun 8, 2024

draft status for > 30 days

@andrewrk andrewrk closed this Jun 8, 2024
@mlugg
Copy link
Member

mlugg commented Jun 8, 2024

(Succeeded by #19812, which I will finish off soon!)

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.

introduce labeled continue syntax inside a switch expression
6 participants