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 10 commits into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

tracking issue: #138777
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

@@ -244,6 +248,188 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None
})
}
ExprKind::LoopMatch { state, region_scope, ref arms, .. } => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the body of this match arm should be broken out and located elsewhere, in the same way that match_expr is, and should have a substantial doc comment above it, like match_expr does.

Comment on lines 746 to 750
let rustc_middle::thir::ExprKind::Scope { value, .. } =
self.thir[value.unwrap()].kind
else {
panic!();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is unreachable, it's worth marking it that way. Either way, it's worth a comment about this.

Comment on lines +846 to +909
//let is_coroutine = this.coroutine.is_some();

/*// Link the exit drop tree to unwind drop tree.
if drops.drops.iter().any(|drop_node| drop_node.data.kind == DropKind::Value) {
let unwind_target = this.diverge_cleanup_target(region_scope, span);
let mut unwind_indices = IndexVec::from_elem_n(unwind_target, 1);
for (drop_idx, drop_node) in drops.drops.iter_enumerated().skip(1) {
match drop_node.data.kind {
DropKind::Storage | DropKind::ForLint => {
if is_coroutine {
let unwind_drop = this.scopes.unwind_drops.add_drop(
drop_node.data,
unwind_indices[drop_node.next],
);
unwind_indices.push(unwind_drop);
} else {
unwind_indices.push(unwind_indices[drop_node.next]);
}
}
DropKind::Value => {
let unwind_drop = this
.scopes
.unwind_drops
.add_drop(drop_node.data, unwind_indices[drop_node.next]);
this.scopes.unwind_drops.add_entry_point(
blocks[drop_idx].unwrap(),
unwind_indices[drop_node.next],
);
unwind_indices.push(unwind_drop);
}
}
}
}*/
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the story here?

Copy link
Member

@bjorn3 bjorn3 Mar 22, 2025

Choose a reason for hiding this comment

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

Dropping the rest of the local variables when unwinding out of drops on #[const_continue] isn't implemented yet.

@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

Comment on lines +95 to +100
ExprKind::ConstContinue { label, value } => this.break_scope(
block,
Some(value),
BreakableTarget::ConstContinue(label),
source_info,
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bjorn3 just noticing, this should probably get its own function. it does not really share any logic with the others in break_scope at all (currently anyway)

@bors
Copy link
Contributor

bors commented Mar 26, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) 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.

6 participants