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
Contributor

Nadrieril commented Nov 12, 2019

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 12, 2019

r? @varkor

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

@Nadrieril

This comment has been minimized.

Copy link
Contributor Author

Nadrieril commented Nov 12, 2019

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 {

This comment has been minimized.

Copy link
@Centril

Centril Nov 12, 2019

Member

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

This comment has been minimized.

Copy link
Member

Centril commented Nov 12, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 12, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

Nadrieril commented Nov 12, 2019

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

This comment has been minimized.

Copy link
Member

varkor commented Nov 12, 2019

Does this fix #53708 as well?

@Nadrieril

This comment has been minimized.

Copy link
Contributor Author

Nadrieril commented Nov 12, 2019

@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 left a comment

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

This comment has been minimized.

Copy link
Member

Centril commented Nov 13, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 13, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

bors commented Nov 13, 2019

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 13, 2019

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 13, 2019

Finished benchmarking try commit 9fabb30, comparison URL.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 14, 2019

@bors rollup=never

@Nadrieril Nadrieril force-pushed the Nadrieril:refactor-intrange branch from 0e3ec65 to 694a511 Nov 15, 2019
@Nadrieril

This comment has been minimized.

Copy link
Contributor Author

Nadrieril commented Nov 15, 2019

Rebased onto master and applied latest reviews :)

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Nov 15, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 15, 2019

📌 Commit 694a511 has been approved by varkor

@bors

This comment has been minimized.

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

This comment has been minimized.

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 label Nov 16, 2019
@bors bors merged commit 694a511 into rust-lang:master Nov 16, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191115.35 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@Nadrieril Nadrieril deleted the Nadrieril:refactor-intrange branch Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.