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

Refactor integer range handling in the usefulness algorithm #66326

Merged
merged 29 commits into from
Nov 16, 2019

Conversation

Nadrieril
Copy link
Member

Integer range handling had accumulated a lot of debt. This cleans up a lot of it.

In particular this:

  • removes unnecessary conversions between Const and u128, and between Constructor and IntRange
  • clearly distinguishes between on the one hand ranges of integers that may or may not be matched exhaustively, and on the other hand ranges of non-integers that are never matched exhaustively and are compared using Const-based shenanigans
  • cleans up some overly complicated code paths
  • generally tries to be more idiomatic.

As a nice side-effect, I measured a 10% perf increase on unicode_normalization.

There's one thing that I feel remains to clean up: the overlapping range check, which is currently quite ad-hoc. But that is intricate enough that I'm leaving it out of this PR.

There's also one little thing I'm not sure I understand: can try_eval_bits fail for an integer constant value in that code ? What would that mean, and how do I construct a test case for this possibility ?

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2019
@Nadrieril
Copy link
Member Author

Actually I figured out a test case where try_eval_bits fails: 1084eff . However I still don't understand what's causing this and whether this is a feature or a bug.

src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
@@ -1418,6 +1427,12 @@ impl<'tcx> IntRange<'tcx> {
}
}

fn is_subrange(&self, other: &Self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a useful operation that could be added to the standard library on ranges; cc @dtolnay

src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Nov 12, 2019

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 12, 2019

⌛ Trying commit 1084eff with merge f1da198...

bors added a commit that referenced this pull request Nov 12, 2019
Refactor integer range handling in the usefulness algorithm

Integer range handling had accumulated a lot of debt. This cleans up a lot of it.

In particular this:
- removes unnecessary conversions between `Const` and `u128`, and between `Constructor` and `IntRange`
- clearly distinguishes between on the one hand ranges of integers that may or may not be matched exhaustively, and on the other hand ranges of non-integers that are never matched exhaustively and are compared using Const-based shenanigans
- cleans up some overly complicated code paths
- generally tries to be more idiomatic.

As a nice side-effect, I measured a 10% perf increase on `unicode_normalization`.

There's one thing that I feel remains to clean up: the [overlapping range check](#64007), which is currently quite ad-hoc. But that is intricate enough that I'm leaving it out of this PR.

There's also one little thing I'm not sure I understand: can `try_eval_bits` fail for an integer constant value in that code ? What would that mean, and how do I construct a test case for this possibility ?
@Nadrieril
Copy link
Member Author

Hmm, it turns out I unknowingly fixed a bug:

const BAR: &i32 = &42;
match &0 {
    BAR => {}
    _ => {}
}

used to flag the first branch as unreachable. This looks similar to #65413.

@varkor
Copy link
Member

varkor commented Nov 12, 2019

Does this fix #53708 as well?

@Nadrieril
Copy link
Member Author

@varkor No, this was a bug specific to integer ranges and values, in the case where try_eval_bits fails. There is a link with #53708 however: in both cases the problem comes from consts behind references.

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

I like what you've done here. Just have a couple of very minor comments :)

src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Nov 13, 2019

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 13, 2019

⌛ Trying commit 0e3ec65 with merge 9fabb30...

bors added a commit that referenced this pull request Nov 13, 2019
Refactor integer range handling in the usefulness algorithm

Integer range handling had accumulated a lot of debt. This cleans up a lot of it.

In particular this:
- removes unnecessary conversions between `Const` and `u128`, and between `Constructor` and `IntRange`
- clearly distinguishes between on the one hand ranges of integers that may or may not be matched exhaustively, and on the other hand ranges of non-integers that are never matched exhaustively and are compared using Const-based shenanigans
- cleans up some overly complicated code paths
- generally tries to be more idiomatic.

As a nice side-effect, I measured a 10% perf increase on `unicode_normalization`.

There's one thing that I feel remains to clean up: the [overlapping range check](#64007), which is currently quite ad-hoc. But that is intricate enough that I'm leaving it out of this PR.

There's also one little thing I'm not sure I understand: can `try_eval_bits` fail for an integer constant value in that code ? What would that mean, and how do I construct a test case for this possibility ?
@bors
Copy link
Contributor

bors commented Nov 13, 2019

☀️ Try build successful - checks-azure
Build commit: 9fabb30 (9fabb30244907581fb04a64c9cb27f3ff9db04fc)

@rust-timer
Copy link
Collaborator

Queued 9fabb30 with parent 374ad1b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9fabb30, comparison URL.

@Centril
Copy link
Contributor

Centril commented Nov 14, 2019

@bors rollup=never

@Nadrieril
Copy link
Member Author

Rebased onto master and applied latest reviews :)

@varkor
Copy link
Member

varkor commented Nov 15, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 15, 2019

📌 Commit 694a511 has been approved by varkor

@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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2019
@bors
Copy link
Contributor

bors commented Nov 15, 2019

⌛ Testing commit 694a511 with merge 82161cd...

bors added a commit that referenced this pull request Nov 15, 2019
Refactor integer range handling in the usefulness algorithm

Integer range handling had accumulated a lot of debt. This cleans up a lot of it.

In particular this:
- removes unnecessary conversions between `Const` and `u128`, and between `Constructor` and `IntRange`
- clearly distinguishes between on the one hand ranges of integers that may or may not be matched exhaustively, and on the other hand ranges of non-integers that are never matched exhaustively and are compared using Const-based shenanigans
- cleans up some overly complicated code paths
- generally tries to be more idiomatic.

As a nice side-effect, I measured a 10% perf increase on `unicode_normalization`.

There's one thing that I feel remains to clean up: the [overlapping range check](#64007), which is currently quite ad-hoc. But that is intricate enough that I'm leaving it out of this PR.

There's also one little thing I'm not sure I understand: can `try_eval_bits` fail for an integer constant value in that code ? What would that mean, and how do I construct a test case for this possibility ?
@bors
Copy link
Contributor

bors commented Nov 16, 2019

☀️ Test successful - checks-azure
Approved by: varkor
Pushing 82161cd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 16, 2019
@bors bors merged commit 694a511 into rust-lang:master Nov 16, 2019
@Nadrieril Nadrieril deleted the refactor-intrange branch November 16, 2019 13:24
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants