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

Add #[loop_match] for improved DFA codegen #138780

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

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Mar 21, 2025

tracking issue: #132306
project goal: rust-lang/rust-project-goals#258

This PR adds the #[loop_match] attribute, which aims to improve code generation for state machines. For some (very exciting) benchmarks, see rust-lang/rust-project-goals#258 (comment)

Currently, a very restricted syntax pattern is accepted. We'd like to get feedback and merge this now before we go too far in a direction that others have concerns with.

current state

We accept code that looks like this

#[loop_match]
loop {
    state = 'blk: {
        match state {
            State::A => {
                #[const_continue]
                break 'blk State::B
            }
            State::B => { /* ... */ }
            /* ... */
        }
    }
}
  • a loop should have the same semantics with and without #[loop_match]: normal continue and break continue to work
  • #[const_continue] is only allowed in loops annotated with #[loop_match]`
  • the loop body needs to have this particular shape (a single assignment to the match scrutinee, with the body a labelled block containing just a match)

future work

  • perform const evaluation on the break value
  • support more state/scrutinee types
  • report proper errors when the jump target could not be determined statically
  • rework the pattern matching logic so hopefully it can use more existing code

maybe future work

  • allow continue 'label value syntax, which #[const_continue] could then use.
  • allow the match to be on an arbitrary expression (e.g. State::Initial)
  • attempt to also optimize break/continue expressions that are not marked with #[const_continue]

r? @traviscross

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2025

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

Copy link
Contributor

@traviscross traviscross left a comment

Choose a reason for hiding this comment

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

Thanks @folkertdev for putting up this PR. The big picture looks right, in terms of the behavior of the tests and how to approach the experiment in terms of starting with the attributes for thiis.

This is a first partial pass on the details.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2025
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor Author

@folkertdev folkertdev left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed review!

I've fixed a bunch of the low-hanging fruit (e.g. in the tests). For the actual pattern matching logic, I have a branch with what I believe is a better solution that re-uses more existing pattern matching infra. We'll come back to that here once björn has had a chance to look at it.

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2025

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in match lowering

cc @Nadrieril

@bors
Copy link
Collaborator

bors commented Mar 26, 2025

☔ The latest upstream changes (presumably #138974) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@folkertdev
Copy link
Contributor Author

We've done a bunch of work here, and I believe all of the earlier review comments have now been dealt with.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 4, 2025
Comment on lines +82 to +87
LoopMatch { state, ref arms, .. } => {
visitor.visit_expr(&visitor.thir()[state]);
for &arm in &**arms {
visitor.visit_arm(&visitor.thir()[arm]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's combine this arm with the one for Match below.

}
}

fn static_pattern_match_help(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there's some better name for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. And documentation. I understand this checks whether constant matches pat?

@traviscross
Copy link
Contributor

traviscross commented Apr 6, 2025

@rustbot author

As a lang matter, this is looking reasonable to me in terms of a lang experiment.

As an impl matter, this is starting to look not unreasonable to me, but I'd like for @Nadrieril to also have a look if he's able.

r? @Nadrieril

@Nadrieril: I still need to raise this in a lang meeting to confirm that everyone is happy to see the experiment here in light of earlier objections, so please don't merge this just yet. You can leave it back in my hands after you're happy with the impl.

Also CC @oli-obk as this work is carrying over some FIXME items you have marked.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 6, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot assigned Nadrieril and unassigned traviscross Apr 6, 2025
@traviscross traviscross self-assigned this Apr 6, 2025
@traviscross traviscross added the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 6, 2025
@Nadrieril
Copy link
Member

Only speaking of the MIR lowering part: my opinion on the current implementation is that this is a fine approach for an experiment, but this will need to change in depth before it can be relied on.

For one, I believe the static pattern-matching should use the const-eval interpreter instead of manually operating on valtrees. For two, it should not duplicate the work of match lowering; instead, BuiltMatchTree should track the tests cases that lead to each branch so that we can reuse them. This is the only way or-patterns can be supported properly.

Haven't reviewed the rest, would appreciate any help there, otherwise I'll get to it once approved.

@Nadrieril
Copy link
Member

In terms of the experiment, my current take is that using patterns for this doesn't pull its weight. I expect we won't allow guaranteed-direct-jump using a non-fully-const value like:

#[loop_match]
loop {
    state = 'blk: {
        match state {
            None => {
                let r = random();
                const continue 'blk Some(r);
            },
            Some(_) => break,
        }
    };
}

The reason being that this requires inspecting the expression which is a weird sort of abstraction break (you wouldn't be able to do let x = Some(r); const continue 'blk x;).

And without that, using patterns seems barely better than jump labels.

@traviscross
Copy link
Contributor

traviscross commented Apr 6, 2025

Thanks for having a look at the MIR lowering. That was indeed what I most wanted your eyes on.

For one, I believe the static pattern-matching should use the const-eval interpreter instead of manually operating on valtrees. For two, it should not duplicate the work of match lowering; instead, BuiltMatchTree should track the tests cases that lead to each branch so that we can reuse them. This is the only way or-patterns can be supported properly.

@folkertdev: What are your thoughts on this and on how and when you want to approach it?

In terms of the experiment... I expect we won't allow guaranteed-direct-jump using a non-fully-const value... The reason being that this requires inspecting the expression which is a weird sort of abstraction break (you wouldn't be able to do let x = Some(r); const continue 'blk x;).

The current implementation only supports integers and enums without fields as the scrutinee/state. Clearly we should never accept the abstraction break that you mention.

And without that, using patterns seems barely better than jump labels.

Whether or not we use patterns, what seems fairly elegant to me about this approach is that it keeps the jump labels in the value space which means that const computations can be performed to choose the jump target and that arbitrary other computations can be performed to choose the target when needed by leaving off the #[const_continue] (and accepting the codegen implications of that) without having to switch the entire block to a different syntax or engage in other workarounds.

Of course, I can imagine ways we could keep the jump labels in the value space without using patterns at all (rather than using them in restricted form by restricting the scrutinee type), and it'd be interesting to think through the pros and cons of that.

@bjorn3
Copy link
Member

bjorn3 commented Apr 7, 2025

In terms of the experiment... I expect we won't allow guaranteed-direct-jump using a non-fully-const value... The reason being that this requires inspecting the expression which is a weird sort of abstraction break (you wouldn't be able to do let x = Some(r); const continue 'blk x;).

The current implementation only supports integers and enums without fields as the scrutinee/state. Clearly we should never accept the abstraction break that you mention.

The argument of the const continue is a const value. Either a literal const {} block or for convenience directly an integer or enum literal.

For one, I believe the static pattern-matching should use the const-eval interpreter instead of manually operating on valtrees. For two, it should not duplicate the work of match lowering; instead, BuiltMatchTree should track the tests cases that lead to each branch so that we can reuse them. This is the only way or-patterns can be supported properly.

Const eval needs a fully built MIR body, but we are currently building a MIR body, so there is nothing const eval can run on. As for BuiltMatchTree, that is already used and static_pattern_match handles or patterns already. Everything is just intentionally limited to integers, bools and fieldless enums to make the implementation easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants