Remove dead unwinds before drop elaboration #106430

tmiasko · 2023-01-03T23:58:03Z

As a part of drop elaboration, we identify dead unwinds, i.e., unwind
edges on a drop terminators which are known to be unreachable, because
there is no need to drop anything.

Previously, the data flow framework was informed about the dead unwinds,
and it assumed those edges are absent from MIR. Unfortunately, the data
flow framework wasn't consistent in maintaining this assumption.

In particular, if a block was reachable only through a dead unwind edge,
its state was propagated to other blocks still. This became an issue in
the context of change removes DropAndReplace terminator, since it
introduces initialization into cleanup blocks.

To avoid this issue, remove unreachable unwind edges before the drop
elaboration, and elaborate only blocks that remain reachable.

cc @zeegomo

rustbot · 2023-01-03T23:58:09Z

r? @Nilstrieb

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

rustbot · 2023-01-03T23:58:11Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

tmiasko · 2023-01-04T00:16:29Z

@bors try @rust-timer queue

bors · 2023-01-04T00:16:37Z

⌛ Trying commit e80481c77ab081849f4e93f0f7e2547d9fca728a with merge 28a389c3fbb6dffe42f0f3257472197153ef82a7...

bors · 2023-01-04T03:00:27Z

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

rust-timer · 2023-01-04T05:36:11Z

Finished benchmarking commit (28a389c3fbb6dffe42f0f3257472197153ef82a7): comparison URL.

Overall result: no relevant changes - no 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.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

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

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)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-	-	0

zeegomo

That's cool indeed, thanks!

compiler/rustc_mir_transform/src/elaborate_drops.rs

tmiasko · 2023-01-04T22:52:28Z

@bors try @rust-timer queue

bors · 2023-01-04T22:52:36Z

⌛ Trying commit dc136d1f6b41424db991463ee3ec4f0a990ee03e with merge b5371b715c5c66f73e977d7b4329bbd5bfeacb61...

bors · 2023-01-05T01:42:09Z

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

rust-timer · 2023-01-05T03:43:59Z

Finished benchmarking commit (b5371b715c5c66f73e977d7b4329bbd5bfeacb61): 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-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.7%	[0.3%, 1.1%]	28
Regressions ❌ (secondary)	0.8%	[0.3%, 1.3%]	26
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.3%, 1.1%]	28

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

Cycles

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

Noratrieb · 2023-01-05T09:40:54Z

r? mir-opt

bors · 2023-02-24T05:38:55Z

⌛ Testing commit 2a70524 with merge 7d68ab0858e4dc6a33bc9a237287b61eb2f99671...

bors · 2023-02-24T09:53:55Z

💥 Test timed out

WaffleLapkin · 2023-02-24T11:56:52Z

@bors retry

rust-log-analyzer · 2023-02-24T14:29:59Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-02-25T02:50:01Z

⌛ Testing commit 2a70524 with merge e08ad60cc85327cd0f7eadf1450b622cd2a0774e...

bors · 2023-02-25T03:01:33Z

💔 Test failed - checks-actions

ehuss · 2023-02-25T03:09:35Z

@bors retry

#108227

rust-log-analyzer · 2023-02-25T04:44:42Z

The job x86_64-mingw-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[2541/3025] Linking CXX static library lib\libLLVMSymbolize.a
[2542/3025] Linking CXX static library lib\libLLVMObjectYAML.a
[2543/3025] Linking CXX static library lib\libLLVMDebuginfod.a
[2544/3025] Linking CXX static library lib\libLLVMProfileData.a
FAILED: lib/libLLVMProfileData.a 
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E rm -f lib\libLLVMProfileData.a && D:\a\rust\rust\mingw64\bin\ar.exe qc lib\libLLVMProfileData.a  lib/ProfileData/CMakeFiles/LLVMProfileData.dir/GCOV.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/InstrProf.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/InstrProfCorrelator.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/InstrProfReader.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/InstrProfWriter.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/MemProf.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/ProfileSummaryBuilder.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/SampleProf.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/SampleProfReader.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/SampleProfWriter.cpp.obj lib/ProfileData/CMakeFiles/LLVMProfileData.dir/RawMemProfReader.cpp.obj && D:\a\rust\rust\mingw64\bin\ranlib.exe lib\libLLVMProfileData.a && cd ."
D:\a\rust\rust\mingw64\bin\ar.exe: could not create temporary file whilst writing archive: no more archived files
[2545/3025] Building CXX object lib/XRay/CMakeFiles/LLVMXRay.dir/Profile.cpp.obj
[2546/3025] Building CXX object lib/XRay/CMakeFiles/LLVMXRay.dir/RecordInitializer.cpp.obj
[2547/3025] Building CXX object lib/XRay/CMakeFiles/LLVMXRay.dir/LogBuilderConsumer.cpp.obj
[2548/3025] Building CXX object lib/WindowsManifest/CMakeFiles/LLVMWindowsManifest.dir/WindowsManifestMerger.cpp.obj
[2548/3025] Building CXX object lib/WindowsManifest/CMakeFiles/LLVMWindowsManifest.dir/WindowsManifestMerger.cpp.obj
[2549/3025] Building CXX object utils/FileCheck/CMakeFiles/FileCheck.dir/FileCheck.cpp.obj
[2550/3025] Building CXX object utils/PerfectShuffle/CMakeFiles/llvm-PerfectShuffle.dir/PerfectShuffle.cpp.obj
[2551/3025] Building CXX object lib/WindowsDriver/CMakeFiles/LLVMWindowsDriver.dir/MSVCPaths.cpp.obj
ninja: build stopped: subcommand failed.
command did not execute successfully, got: exit code: 1


build script failed, must exit now', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cmake-0.1.48\src\lib.rs:975:5
 finished in 220.609 seconds
Build completed unsuccessfully in 0:06:00
Build completed unsuccessfully in 0:06:00
make: *** [Makefile:78: ci-mingw-subset-1] Error 1

bors · 2023-02-25T06:14:38Z

⌛ Testing commit 2a70524 with merge 313219c...

bors · 2023-02-25T09:17:00Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 313219c to master...

rust-timer · 2023-02-25T10:34:21Z

Finished benchmarking commit (313219c): 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)	3.0%	[3.0%, 3.0%]	1
Regressions ❌ (secondary)	4.7%	[4.7%, 4.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[3.0%, 3.0%]	1

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)	5.1%	[3.9%, 6.2%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Desugaring of drop and replace at MIR build This commit desugars the drop and replace deriving from an assignment at MIR build, avoiding the construction of the `DropAndReplace` terminator (which will be removed in a following PR). In order to retain the same error messages for replaces a new `DesugaringKind::Replace` variant is introduced. The changes in the borrowck are also useful for future work in moving drop elaboration before borrowck, as no `DropAndReplace` would be present there anymore. Notes on test diffs: * `tests/ui/borrowck/issue-58776-borrowck-scans-children`: the assignment deriving from the desugaring kills the borrow. * `tests/ui/async-await/async-fn-size-uninit-locals.rs`, `tests/mir-opt/issue_41110.test.ElaborateDrops.after.mir`, `tests/mir-opt/issue_41888.main.ElaborateDrops.after.mir`: drop elaboration generates (or reads from) a useless drop flag due to an issue with the dataflow analysis. Will be fixed independently by rust-lang#106430. See rust-lang#104488 for more context

Desugaring of drop and replace at MIR build This commit desugars the drop and replace deriving from an assignment at MIR build, avoiding the construction of the `DropAndReplace` terminator (which will be removed in a following PR). In order to retain the same error messages for replaces a new `DesugaringKind::Replace` variant is introduced. The changes in the borrowck are also useful for future work in moving drop elaboration before borrowck, as no `DropAndReplace` would be present there anymore. Notes on test diffs: * `tests/ui/borrowck/issue-58776-borrowck-scans-children`: the assignment deriving from the desugaring kills the borrow. * `tests/ui/async-await/async-fn-size-uninit-locals.rs`, `tests/mir-opt/issue_41110.test.ElaborateDrops.after.mir`, `tests/mir-opt/issue_41888.main.ElaborateDrops.after.mir`: drop elaboration generates (or reads from) a useless drop flag due to an issue with the dataflow analysis. Will be fixed independently by rust-lang/rust#106430. See rust-lang/rust#104488 for more context

rustbot assigned Noratrieb Jan 3, 2023

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 Jan 3, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 4, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 4, 2023

zeegomo reviewed Jan 4, 2023

View reviewed changes

compiler/rustc_mir_transform/src/elaborate_drops.rs Outdated Show resolved Hide resolved

compiler/rustc_mir_transform/src/elaborate_drops.rs Show resolved Hide resolved

tmiasko force-pushed the rm-dead-unwinds branch from e80481c to dc136d1 Compare January 4, 2023 22:22

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 4, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 5, 2023

tmiasko force-pushed the rm-dead-unwinds branch from dc136d1 to 4e2092b Compare January 5, 2023 09:35

rustbot assigned wesleywiser and unassigned Noratrieb Jan 5, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2023

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 Feb 24, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 25, 2023

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 Feb 25, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 25, 2023

bors merged commit 313219c into rust-lang:master Feb 25, 2023

rustbot added this to the 1.69.0 milestone Feb 25, 2023

tmiasko deleted the rm-dead-unwinds branch February 25, 2023 09:51

bors mentioned this pull request Feb 25, 2023

Take MIR dataflow analyses by mutable reference #108293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dead unwinds before drop elaboration #106430

Remove dead unwinds before drop elaboration #106430

tmiasko commented Jan 3, 2023

rustbot commented Jan 3, 2023

rustbot commented Jan 3, 2023

tmiasko commented Jan 4, 2023

This comment has been minimized.

bors commented Jan 4, 2023

bors commented Jan 4, 2023

This comment has been minimized.

rust-timer commented Jan 4, 2023

zeegomo left a comment

tmiasko commented Jan 4, 2023

This comment has been minimized.

bors commented Jan 4, 2023

bors commented Jan 5, 2023

This comment has been minimized.

rust-timer commented Jan 5, 2023

Noratrieb commented Jan 5, 2023

bors commented Feb 24, 2023

bors commented Feb 24, 2023

WaffleLapkin commented Feb 24, 2023

rust-log-analyzer commented Feb 24, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

ehuss commented Feb 25, 2023

rust-log-analyzer commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

rust-timer commented Feb 25, 2023

Remove dead unwinds before drop elaboration #106430

Remove dead unwinds before drop elaboration #106430

Conversation

tmiasko commented Jan 3, 2023

rustbot commented Jan 3, 2023

rustbot commented Jan 3, 2023

tmiasko commented Jan 4, 2023

This comment has been minimized.

bors commented Jan 4, 2023

bors commented Jan 4, 2023

This comment has been minimized.

rust-timer commented Jan 4, 2023

Overall result: no relevant changes - no action needed

zeegomo left a comment

Choose a reason for hiding this comment

tmiasko commented Jan 4, 2023

This comment has been minimized.

bors commented Jan 4, 2023

bors commented Jan 5, 2023

This comment has been minimized.

rust-timer commented Jan 5, 2023

Overall result: ❌ regressions - ACTION NEEDED

Noratrieb commented Jan 5, 2023

bors commented Feb 24, 2023

bors commented Feb 24, 2023

WaffleLapkin commented Feb 24, 2023

rust-log-analyzer commented Feb 24, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

ehuss commented Feb 25, 2023

rust-log-analyzer commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

rust-timer commented Feb 25, 2023

Overall result: no relevant changes - no action needed