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

match lowering: sort Eq candidates in the failure case too #122459

Merged
merged 4 commits into from Mar 31, 2024

Conversation

Nadrieril
Copy link
Member

This is a slight tweak to MIR gen of matches. Take a match like:

match (s, flag) {
    ("a", _) if foo() => 1,
    ("b", true) => 2,
    ("a", false) => 3,
    (_, true) => 4,
    _ => 5,
}

If we switch on s == "a", the first candidate matches, and we learn almost nothing about the second candidate. So there's a choice:

  1. (what we do today) stop sorting candidates, keep the "b" case grouped with everything below. This could allow us to be clever here and test on flag == true next.
  2. (what this PR does) sort "b" into the failure case. The "b" will be alone (fewer opportunities for picking a good test), but that means the two "a" cases require a single test.

Today, we aren't clever in which tests we pick, so this is an unambiguous win. In a future where we pick tests better, idk. Grouping tests as much as possible feels like a generally good strategy.

This was proposed in #29623 (9 years ago :D)

@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added 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 13, 2024
} else {
fully_matched = false;
Some(TestBranch::Failure)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

fully_matched = test_val == case_val;
Some(if fully_matched { TestBranch::Success } else { TestBranch::Failure })

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer keeping them separate because the fully_matched logic is already not easy to follow across the function

@petrochenkov
Copy link
Contributor

Is there anybody who reviews pattern matching changes regularly?
I have no idea what "sorting" and "grouping" mean in this context, and what are implications of the change, so it's probably better to reassign.
@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 14, 2024
@Nadrieril
Copy link
Member Author

I know one but I was hoping I could give the easy changes to others. Let's see...

r? @oli-obk do you follow the match lowering logic?

@rustbot rustbot assigned oli-obk and unassigned petrochenkov Mar 14, 2024
@@ -0,0 +1,32 @@
// skip-filecheck
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're looking for specific patterns, it may be good to use filecheck to ensure it doesn't get blessed away in some larger change

@Nadrieril
Copy link
Member Author

@bors r=@oli-obk

@bors
Copy link
Contributor

bors commented Mar 31, 2024

📌 Commit 65efa5b has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 31, 2024
@bors
Copy link
Contributor

bors commented Mar 31, 2024

⌛ Testing commit 65efa5b with merge 5baf1e1...

@bors
Copy link
Contributor

bors commented Mar 31, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 5baf1e1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 31, 2024
@bors bors merged commit 5baf1e1 into rust-lang:master Mar 31, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 31, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5baf1e1): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-5.9% [-5.9%, -5.9%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -5.9% [-5.9%, -5.9%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.4% [2.4%, 4.7%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.3% [-0.3%, -0.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.3% [-0.3%, -0.3%] 1

Bootstrap: 667.618s -> 670.332s (0.41%)
Artifact size: 315.66 MiB -> 315.71 MiB (0.02%)

@Nadrieril Nadrieril deleted the sort-eq branch March 31, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

None yet

6 participants