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.

Sign up for GitHub

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

Jump to bottom

Replace switch to unreachable by assume statements #113970

Merged
merged 6 commits into from
Nov 1, 2023
Merged

Replace switch to unreachable by assume statements #113970

merged 6 commits into from
Nov 1, 2023

Conversation

cjgillot
Copy link
Contributor

UnreachablePropagation currently keeps some switch terminators alive in order to ensure codegen can infer the inequalities on the discriminants.

This PR proposes to encode those inequalities as Assume statements.

This allows to simplify MIR further by removing some useless terminators.

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2023

r? @wesleywiser

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

@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 Jul 22, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 22, 2023
@bors
Copy link
Contributor

bors commented Jul 22, 2023

⌛ Trying commit 0657bc785d7ffb2c86cf1fce7f7cc0744d5e52ae with merge ad0888358a489103ed6806cffede10c65252400d...

@bors
Copy link
Contributor

bors commented Jul 22, 2023

☀️ Try build successful - checks-actions
Build commit: ad0888358a489103ed6806cffede10c65252400d (ad0888358a489103ed6806cffede10c65252400d)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ad0888358a489103ed6806cffede10c65252400d): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)		 0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)		 -0.4% [-0.5%, -0.4%] 2
Improvements ✅
(secondary)		 -0.4% [-0.4%, -0.4%] 2
All ❌✅ (primary) -0.2% [-0.5%, 0.4%] 3

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)		 4.2% [0.6%, 10.2%] 6
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.6% [-2.7%, -2.5%] 2
All ❌✅ (primary) 4.2% [0.6%, 10.2%] 6

Cycles

This benchmark run did not return any relevant results for this metric.

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.0% [0.0%, 0.1%] 5
Regressions ❌
(secondary)		 0.1% [0.1%, 0.1%] 3
Improvements ✅
(primary)		 -0.1% [-1.0%, -0.0%] 44
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.1% [-1.0%, 0.1%] 49

Bootstrap: 651.047s -> 650.252s (-0.12%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 23, 2023
@cjgillot cjgillot added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations labels Aug 8, 2023
@apiraino
Copy link
Contributor

@cjgillot a comment on the last perf run? Apart from that this seems ready for review, thanks!

@rustbot ready

@cjgillot
Copy link
Contributor Author

Perf : small improvements and regressions balance out.

@apiraino
Copy link
Contributor

r? compiler

@rustbot rustbot assigned jackh726 and unassigned wesleywiser Aug 31, 2023
@cjgillot cjgillot force-pushed the assume-all-the-things branch from 0657bc7 to 930edfa Compare August 31, 2023 16:37
@apiraino
Copy link
Contributor

r? compiler

@rustbot rustbot assigned petrochenkov and unassigned jackh726 Sep 21, 2023
@petrochenkov
Copy link
Contributor

r? compiler

@rustbot rustbot assigned b-naber and unassigned petrochenkov Sep 21, 2023
@cjgillot cjgillot force-pushed the assume-all-the-things branch from 930edfa to aa7e5ca Compare September 21, 2023 23:57
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 9, 2023

☔ The latest upstream changes (presumably #116569) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot cjgillot force-pushed the assume-all-the-things branch 2 times, most recently from 9ad64b9 to 631ce68 Compare October 21, 2023 14:07
@apiraino
Copy link
Contributor

This PR has received 2 r+ (here and here). Can it be merged or is there more work to do (or comments to be addressed)?

@cjgillot
Copy link
Contributor Author

The tests failed repeatedly. At this point, I have not found a cause, but I cannot rule out this PR causes them.

@cjgillot cjgillot force-pushed the assume-all-the-things branch from 631ce68 to ae2e211 Compare October 31, 2023 22:39
@cjgillot
Copy link
Contributor Author

Rebased. Let's see if the failure is gone.
@bors r=nikic

@bors
Copy link
Contributor

bors commented Oct 31, 2023

📌 Commit ae2e211 has been approved by nikic

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

bors commented Nov 1, 2023

⌛ Testing commit ae2e211 with merge 98f5ebb...

@bors
Copy link
Contributor

bors commented Nov 1, 2023

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 98f5ebb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 1, 2023
@bors bors merged commit 98f5ebb into rust-lang:master Nov 1, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 1, 2023
@bors bors mentioned this pull request Nov 1, 2023
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (98f5ebb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)		 4.0% [2.5%, 5.6%] 4
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.7% [-2.8%, -0.1%] 5
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.8% [-2.8%, 5.6%] 9

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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -6.8% [-6.8%, -6.8%] 1
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.0% [0.0%, 0.1%] 8
Regressions ❌
(secondary)		 0.1% [0.1%, 0.1%] 3
Improvements ✅
(primary)		 -0.1% [-0.6%, -0.0%] 39
Improvements ✅
(secondary)		 -0.0% [-0.0%, -0.0%] 1
All ❌✅ (primary) -0.1% [-0.6%, 0.1%] 47

Bootstrap: 638.855s -> 636.96s (-0.30%)
Artifact size: 304.47 MiB -> 304.45 MiB (-0.01%)

@rustbot rustbot removed the perf-regression Performance regression. label Nov 1, 2023
@github-actions github-actions bot mentioned this pull request Nov 1, 2023
Merged
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 1, 2023
@bors-ferrocene @poliorcetics
@estebank @dtolnay @celinval @compiler-errors @bors @cjgillot @lcnr @Zalathar @oli-obk
Merge #78
075dd45 
78: Automated pull from upstream `master` r=tshepang a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#113970
* rust-lang/rust#117459
  * rust-lang/rust#117451
  * rust-lang/rust#117439
  * rust-lang/rust#117417
  * rust-lang/rust#117388
  * rust-lang/rust#113241
* rust-lang/rust#117462
* rust-lang/rust#117450
* rust-lang/rust#117407
* rust-lang/rust#117444
  * rust-lang/rust#117438
  * rust-lang/rust#117421
  * rust-lang/rust#117416
  * rust-lang/rust#116712
  * rust-lang/rust#116267
* rust-lang/rust#117377
* rust-lang/rust#117419



Co-authored-by: Alexis (Poliorcetics) Bourget <ab_github@poliorcetiq.eu>
Co-authored-by: Esteban Küber <esteban@kuber.com.ar>
Co-authored-by: David Tolnay <dtolnay@gmail.com>
Co-authored-by: Celina G. Val <celinval@amazon.com>
Co-authored-by: Michael Goulet <michael@errs.io>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Camille GILLOT <gillot.camille@gmail.com>
Co-authored-by: lcnr <rust@lcnr.de>
Co-authored-by: Zalathar <Zalathar@users.noreply.github.com>
Co-authored-by: Oli Scherer <git-spam-no-reply9815368754983@oli-obk.de>
@cjgillot cjgillot deleted the assume-all-the-things branch November 1, 2023 11:03
@cjgillot cjgillot mentioned this pull request Jan 4, 2024
Open
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

@b-naber b-naber b-naber approved these changes

Assignees

@b-naber b-naber

Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations 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
Milestone
1.75.0
Development

Successfully merging this pull request may close these issues.