-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
base: master
Are you sure you want to change the base?
Add #[loop_match]
for improved DFA codegen
#138780
Conversation
Some changes occurred in match checking cc @Nadrieril Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in cc @BoxyUwU |
There was a problem hiding this 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
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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.
Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in match lowering cc @Nadrieril |
☔ The latest upstream changes (presumably #138974) made this pull request unmergeable. Please resolve the merge conflicts. |
368f722
to
a89dcbe
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f294773
to
6fe6909
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Folkert de Vries <[email protected]>
Co-authored-by: Travis Cross <[email protected]>
b3a87ed
to
7d88da4
Compare
We've done a bunch of work here, and I believe all of the earlier review comments have now been dealt with. @rustbot ready |
LoopMatch { state, ref arms, .. } => { | ||
visitor.visit_expr(&visitor.thir()[state]); | ||
for &arm in &**arms { | ||
visitor.visit_arm(&visitor.thir()[arm]); | ||
} | ||
} |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
@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 |
Reminder, once the PR becomes ready for a review, use |
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, Haven't reviewed the rest, would appreciate any help there, otherwise I'll get to it once approved. |
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 And without that, using patterns seems barely better than jump labels. |
Co-authored-by: Travis Cross <[email protected]>
e92034b
to
043be33
Compare
Thanks for having a look at the MIR lowering. That was indeed what I most wanted your eyes on.
@folkertdev: What are your thoughts on this and on how and when you want to approach it?
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.
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 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. |
The argument of the const continue is a const value. Either a literal
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 |
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]
: normalcontinue
andbreak
continue to work#[const_continue] is only allowed in loops annotated with
#[loop_match]`future work
break
valuemaybe future work
continue 'label value
syntax, which#[const_continue]
could then use.State::Initial
)break
/continue
expressions that are not marked with#[const_continue]
r? @traviscross