Cleanup number handling in match exhaustiveness by Nadrieril · Pull Request #116...
source link: https://github.com/rust-lang/rust/pull/116281
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Cleanup number handling in match exhaustiveness #116281
Conversation
Contributor
Doing a little bit of cleanup; handling number constants was somewhat messy. In particular, this:
- evals float consts once instead of repetitively
- reduces
Constructorfrom 88 bytes to 56 (mir::Constis big!)
The fast_try_eval_bits function was mostly constructed from inlining existing code but I don't fully understand it; I don't follow how consts work and are evaluated very well.
Collaborator
|
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
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
Contributor
Author
This comment has been minimized.
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Contributor
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (6d6e3aa): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesResults Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.26s -> 632.231s (-0.00%) |
removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
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
Contributor
Author
|
Let's check if @bors try @rust-timer queue |
This comment has been minimized.
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
This comment has been minimized.
Contributor
Author
|
Oops, left some warnings @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (1451ae6): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 630.889s -> 631.12s (0.04%) |
removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Contributor
|
Thanks! |
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
Contributor
|
☀️ Test successful - checks-actions |
Collaborator
|
Finished benchmarking commit (e0d7ed1): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 627.563s -> 629.592s (0.32%) |
match self { |
||
// If the constant is already evaluated, we shortcut here. |
||
Const::Ty(c) if let ty::ConstKind::Value(valtree) = c.kind() => { |
||
valtree.try_to_scalar_int() |
Are you sure this helps? self.eval will return immediately for ty::ConstKind::Value.
Contributor
Author
I measured a small change on one benchmark yeah. I think looking through the code it always goes through a query
Ah, there's the valtree_to_const_val. But then we should really have this as a fast-path in try_eval_scalar and it should apply on all Const::Ty, not just for things that are already valtrees.
Contributor
Author
I considered it, but the optimization is only correct if we know the normal path will only find Valtree::Leaf I think. We talked about it here #116281 (comment)
Contributor
Author
I mostly felt out of my depth on this ngl
Such discussion should definitely have resulted in a comment in the code...
This is still a change in behavior. Previously, try_eval_scalar_int would have worked on struct newtypes around integers; now it might return None since those are branch valtrees (under our current representation).
Also, previously if this accidentally got called on a reference it would have returned None, but now when called on &i32 it will return the integer value. That seems bad.
Contributor
Author
Oh that's not good at all. I did run all tests once with an assert checking that it's only called on numeric types fwiw. Also there was almost no difference when I loved it into try_eval_scalar_int. We could leave it in deconstruct_pat and document the choices made
Member
I did run all tests once with an assert checking that it's only called on numeric types fwiw.
That doesn't help guard against new uses introduced tomorrow.
#116457 should fix this.
Contributor
Author
Ah yeah that's much better
bors-ferrocene bot
added a commit to ferrocene/ferrocene that referenced this pull request
bors-ferrocene bot
added a commit to ferrocene/ferrocene that referenced this pull request
bors-ferrocene bot
added a commit to ferrocene/ferrocene that referenced this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK