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 unwind in MIR #102906

Merged
merged 16 commits into from
Apr 7, 2023
Merged

Refactor unwind in MIR #102906

merged 16 commits into from
Apr 7, 2023

Conversation

nbdd0121
Copy link
Contributor

This makes unwinding from current Option<BasicBlock> into

enum UnwindAction {
	Continue,
	Cleanup(BasicBlock),
	Unreachable,
	Terminate,
}

cc @JakobDegen @RalfJung @Amanieu

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 11, 2022
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(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 Oct 11, 2022
@JakobDegen
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 11, 2022
@bors
Copy link
Contributor

bors commented Oct 11, 2022

⌛ Trying commit 29a7d5a9230feef9e0a9f019b25ddfa1089bf8e8 with merge 02e51c2b883c3f2fc8a03d5a87b911728cb867bc...

@JakobDegen
Copy link
Contributor

Will definitely review, but might be a couple days

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 11, 2022

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

@rust-timer
Copy link
Collaborator

Queued 02e51c2b883c3f2fc8a03d5a87b911728cb867bc with parent 36c8e29, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (02e51c2b883c3f2fc8a03d5a87b911728cb867bc): 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-review -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.

mean1 range count2
Regressions ❌
(primary)
1.4% [0.6%, 2.0%] 3
Regressions ❌
(secondary)
2.2% [1.1%, 3.9%] 12
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
-1.0% [-1.0%, -1.0%] 1
All ❌✅ (primary) 0.9% [-0.4%, 2.0%] 4

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.

mean1 range count2
Regressions ❌
(primary)
2.8% [1.6%, 4.1%] 9
Regressions ❌
(secondary)
2.6% [1.5%, 6.4%] 50
Improvements ✅
(primary)
-2.4% [-4.7%, -0.2%] 2
Improvements ✅
(secondary)
-4.4% [-4.4%, -4.4%] 1
All ❌✅ (primary) 1.9% [-4.7%, 4.1%] 11

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.

mean1 range count2
Regressions ❌
(primary)
2.3% [2.0%, 2.7%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-3.2%, -2.1%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.2% [-3.2%, 2.7%] 4

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 11, 2022
@RalfJung
Copy link
Member

I only looked at the interpreter changes, those look good modulo the comments I left.

Miri fails to build; ./x.py test src/tools/miri --stage 0 should let you work that out locally.

@Dylan-DPC
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 11, 2022
@bors
Copy link
Contributor

bors commented Oct 11, 2022

⌛ Trying commit 29a7d5a9230feef9e0a9f019b25ddfa1089bf8e8 with merge a8b66909b50a4a69527d99b7213f97309bd55514...

@bors
Copy link
Contributor

bors commented Oct 11, 2022

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

@rust-timer
Copy link
Collaborator

Queued a8b66909b50a4a69527d99b7213f97309bd55514 with parent 3655784, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a8b66909b50a4a69527d99b7213f97309bd55514): comparison URL.

Overall result: ❌ regressions - 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-review -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.

mean1 range count2
Regressions ❌
(primary)
1.7% [0.7%, 2.4%] 3
Regressions ❌
(secondary)
2.1% [1.0%, 3.8%] 12
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.7% [-0.7%, -0.7%] 1
All ❌✅ (primary) 1.7% [0.7%, 2.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.

mean1 range count2
Regressions ❌
(primary)
3.2% [3.2%, 3.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.8% [-6.5%, -0.2%] 8
Improvements ✅
(secondary)
-2.7% [-5.6%, -0.8%] 63
All ❌✅ (primary) -2.1% [-6.5%, 3.2%] 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.

mean1 range count2
Regressions ❌
(primary)
2.8% [2.7%, 3.0%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.7% [-1.9%, -1.5%] 2
Improvements ✅
(secondary)
-3.1% [-4.2%, -2.0%] 2
All ❌✅ (primary) 1.3% [-1.9%, 3.0%] 6

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@tmiasko
Copy link
Contributor

tmiasko commented Apr 7, 2023

@bors delegate=nbdd0121

@bors
Copy link
Contributor

bors commented Apr 7, 2023

✌️ @nbdd0121 can now approve this pull request

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Apr 7, 2023

@bors r=wesleywiser,tmiasko

@bors
Copy link
Contributor

bors commented Apr 7, 2023

📌 Commit ea69dad has been approved by wesleywiser,tmiasko

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 Apr 7, 2023
@bors
Copy link
Contributor

bors commented Apr 7, 2023

⌛ Testing commit ea69dad with merge da14081...

@bors
Copy link
Contributor

bors commented Apr 7, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser,tmiasko
Pushing da14081 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (da14081): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)
1.1% [0.3%, 1.9%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.6%, -0.3%] 10
Improvements ✅
(secondary)
-0.6% [-0.8%, -0.2%] 8
All ❌✅ (primary) -0.2% [-0.6%, 1.9%] 12

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)
7.2% [7.2%, 7.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-4.1%, -0.2%] 5
Improvements ✅
(secondary)
-2.6% [-4.8%, -1.6%] 5
All ❌✅ (primary) -0.8% [-4.1%, 7.2%] 6

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)
1.3% [1.3%, 1.3%] 1
Regressions ❌
(secondary)
3.5% [3.5%, 3.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.3% [1.3%, 1.3%] 1

@nbdd0121 nbdd0121 deleted the mir branch April 7, 2023 19:13
hlisdero added a commit to hlisdero/cargo-check-deadlock that referenced this pull request Apr 8, 2023
…windAction enum.

This is the relevant PR that introduced the changes to rustc
rust-lang/rust#102906

Now there a new enum called `UnwindAction`, which may contain in some cases a basic block for the cleanup that we need to handle.

This change requires some renaming to keep some consistency when using the words "unwind" and "cleanup"

All tests are passing again.

Updated the README
saethlin added a commit to saethlin/miri that referenced this pull request Apr 10, 2023
The panic test is now counted as an error test; we encounter a Terminate
terminator, and emit an interpreter error, as opposed to just
terminating due to a panic. So this test should have broken with
rust-lang/rust#102906 but wasn't because the Miri
test suite is currently broken in rust-lang/rust:
 rust-lang/rust#110102
saethlin added a commit to saethlin/rust that referenced this pull request Apr 11, 2023
The panic test is now counted as an error test; we encounter a Terminate
terminator, and emit an interpreter error, as opposed to just
terminating due to a panic. So this test should have broken with
rust-lang#102906 but wasn't because the Miri
test suite is currently broken in rust-lang/rust:
 rust-lang#110102
@nnethercote
Copy link
Contributor

Perf wins outweigh losses.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 12, 2023
hlisdero added a commit to hlisdero/cargo-check-deadlock that referenced this pull request May 7, 2023
… terminate]`

This is important since a lot of example programs exhibit this structure since the `UnwindAction` enum was introduced to rustc in rust-lang/rust#102906

In a previous commit, the change in rustc was incorporated but without handling all the cases.
This improvement now handles all the variants that
enum `UnwindAction` has instead of ignoring some of them.
8cf95cd
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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.