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

Mark other variants as uninitialized after switch on discriminant #68528

Merged
merged 4 commits into from
Feb 27, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jan 25, 2020

During drop elaboration, which builds the drop ladder that handles destruction during stack unwinding, we attempt to remove MIR Drop terminators that will never be reached in practice. This reduces the number of basic blocks that are passed to LLVM, which should improve performance. In #66753, a user pointed out that unreachable Drop terminators are common in functions like Option::unwrap, which move out of an enum. While discussing possible remedies for that issue, @eddyb suggested moving const-checking after drop elaboration. This would allow the former, which looks for Drop terminators and replicates a small amount of drop elaboration to determine whether a dropped local has been moved out, leverage the work done by the latter.

However, it turns out that drop elaboration is not as precise as it could be when it comes to eliminating useless drop terminators. For example, let's look at the code for unwrap_or.

fn unwrap_or<T>(opt: Option<T>, default: T) -> T {
    match opt {
        Some(inner) => inner,
        None => default,
    }
}

opt never needs to be dropped, since it is either moved out of (if it is Some) or has no drop glue (if it is None), and default only needs to be dropped if opt is Some. This is not reflected in the MIR we currently pass to codegen.

pasted_image

@eddyb also suggested the solution to this problem. When we switch on an enum discriminant, we should be marking all fields in other variants as definitely uninitialized. I implemented this on top of alongside a small optimization (split out into #68943) that suppresses drop terminators for enum variants with no fields (e.g. Option::None). This is the resulting MIR for unwrap_or.

after

In concert with #68943, this change speeds up many optimized and debug builds. We need to carefully investigate whether I have introduced any miscompilations before merging this. Code that never drops anything would be very fast indeed until memory is exhausted.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-01-25T00:29:26.5085565Z ========================== Starting Command Output ===========================
2020-01-25T00:29:26.5090369Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/4cf1a6f3-5c62-48f1-912d-d20290d394d7.sh
2020-01-25T00:29:26.5090656Z 
2020-01-25T00:29:26.5093479Z ##[section]Finishing: Disable git automatic line ending conversion
2020-01-25T00:29:26.5098584Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68528/merge to s
2020-01-25T00:29:26.5100065Z Task         : Get sources
2020-01-25T00:29:26.5100093Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-01-25T00:29:26.5100118Z Version      : 1.0.0
2020-01-25T00:29:26.5100143Z Author       : Microsoft
---
2020-01-25T00:29:27.5029435Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-25T00:29:27.5040134Z ##[command]git config gc.auto 0
2020-01-25T00:29:27.5043486Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-25T00:29:27.5046159Z ##[command]git config --get-all http.proxy
2020-01-25T00:29:27.5052754Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68528/merge:refs/remotes/pull/68528/merge
---
2020-01-25T00:52:49.1214028Z    Compiling rustc_index v0.0.0 (/checkout/src/librustc_index)
2020-01-25T00:52:51.2179474Z error: failed to run custom build command for `backtrace-sys v0.1.32`
2020-01-25T00:52:51.2179565Z 
2020-01-25T00:52:51.2179607Z Caused by:
2020-01-25T00:52:51.2180394Z   process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/release/build/backtrace-sys-227a27b0f872ec81/build-script-build` (signal: 11, SIGSEGV: invalid memory reference)
2020-01-25T00:52:51.2180827Z --- stdout
2020-01-25T00:52:51.2181082Z cargo:rustc-cfg=rbt
2020-01-25T00:52:51.2187011Z TARGET = Some("x86_64-unknown-linux-gnu")
2020-01-25T00:52:51.2187085Z OPT_LEVEL = Some("2")
2020-01-25T00:52:51.2187294Z HOST = Some("x86_64-unknown-linux-gnu")
2020-01-25T00:52:51.2187512Z CC_x86_64-unknown-linux-gnu = Some("sccache cc")
2020-01-25T00:52:51.2187751Z warning: build failed, waiting for other jobs to finish...
2020-01-25T00:52:51.7306709Z error: build failed
2020-01-25T00:52:51.7324740Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "-Zconfig-profile" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2020-01-25T00:52:51.7325391Z expected success, got: exit code: 101
2020-01-25T00:52:51.7325391Z expected success, got: exit code: 101
2020-01-25T00:52:51.7336184Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2020-01-25T00:52:51.7337030Z Build completed unsuccessfully in 0:18:22
2020-01-25T00:52:51.7381164Z == clock drift check ==
2020-01-25T00:52:51.7395424Z   local time: Sat Jan 25 00:52:51 UTC 2020
2020-01-25T00:52:52.0071805Z   network time: Sat, 25 Jan 2020 00:52:52 GMT
2020-01-25T00:52:52.0080702Z == end clock drift check ==
2020-01-25T00:52:52.7690170Z 
2020-01-25T00:52:52.7771012Z ##[error]Bash exited with code '1'.
2020-01-25T00:52:52.7789148Z ##[section]Finishing: Run build
2020-01-25T00:52:52.7800641Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68528/merge to s
2020-01-25T00:52:52.7802137Z Task         : Get sources
2020-01-25T00:52:52.7802172Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-01-25T00:52:52.7802207Z Version      : 1.0.0
2020-01-25T00:52:52.7802254Z Author       : Microsoft
2020-01-25T00:52:52.7802254Z Author       : Microsoft
2020-01-25T00:52:52.7802288Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-01-25T00:52:52.7802325Z ==============================================================================
2020-01-25T00:52:53.1143397Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-01-25T00:52:53.1182947Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/68528/merge to s
2020-01-25T00:52:53.1268109Z Cleaning up task key
2020-01-25T00:52:53.1268730Z Start cleaning up orphan processes.
2020-01-25T00:52:53.1353669Z Terminate orphan process: pid (5563) (python)
2020-01-25T00:52:53.1604548Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@ecstatic-morse
Copy link
Contributor Author

Just to see how slow this makes dataflow:

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jan 25, 2020

⌛ Trying commit 679b4238e793c197eacfe43b7fbc4781e948c36b with merge 3828cd570d1515093538246582353d44c84220db...

@bors
Copy link
Contributor

bors commented Jan 26, 2020

☀️ Try build successful - checks-azure
Build commit: 3828cd570d1515093538246582353d44c84220db (3828cd570d1515093538246582353d44c84220db)

@rust-timer
Copy link
Collaborator

Queued 3828cd570d1515093538246582353d44c84220db with parent 8ad83af, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 3828cd570d1515093538246582353d44c84220db, comparison URL.

@rust-highfive

This comment has been minimized.

@ecstatic-morse ecstatic-morse force-pushed the maybe-init-variants branch 2 times, most recently from aa5dc91 to 64b1424 Compare January 29, 2020 07:21
@ecstatic-morse
Copy link
Contributor Author

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jan 29, 2020

⌛ Trying commit 64b1424dfbf7edd2d3b0e2190fbd85bb93753d8c with merge 0077a7aa11ebc2462851676f9f464d5221b17d6a...

@bors
Copy link
Contributor

bors commented Jan 29, 2020

☀️ Try build successful - checks-azure
Build commit: 0077a7aa11ebc2462851676f9f464d5221b17d6a (0077a7aa11ebc2462851676f9f464d5221b17d6a)

@rust-timer
Copy link
Collaborator

Queued 0077a7aa11ebc2462851676f9f464d5221b17d6a with parent d55f3e9, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 0077a7aa11ebc2462851676f9f464d5221b17d6a, comparison URL.

@bjorn3
Copy link
Member

bjorn3 commented Jan 29, 2020

This reduces the amount of time spent in LLVM. For incremental builds is also reduces the amount of time spent in incr_comp_load_dep_graph by up to 20%. For clean incremental there are a few regressions of ~2%, but those regressions are less than 10ms, so they are likely. The self profiling data even says for some of the regressions that they were actually improvements.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jan 29, 2020

@bjorn3 I added a summary of the changes to the OP, along with an explanation of the perf results for builds that don't involve codegen. They are indeed made slightly slower by this PR. A 10% improvement for syn-opt seems almost too good to be true. In fact, it seems so good that I was hesitant to call attention to these results lest this PR later be found to be unsound. If that improvement is legitimate, I would be overjoyed.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jan 29, 2020

r? @eddyb
cc @rust-lang/wg-mir-opt

Although there's no point in reviewing until #68241 lands.

@eddyb
Copy link
Member

eddyb commented Jan 30, 2020

Since I suggested this approach, I'd rather someone else review:
r? @pnkfelix or @matthewjasper cc @arielb1

@rust-highfive rust-highfive assigned pnkfelix and unassigned eddyb Jan 30, 2020
@hdhoang hdhoang added C-enhancement Category: An issue proposing an enhancement or a PR with one. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #68528!

Tested on commit 49c68bd.
Direct link to PR: #68528

💔 book on windows: test-pass → test-fail (cc @carols10cents @steveklabnik).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Feb 27, 2020
Tested on commit rust-lang/rust@49c68bd.
Direct link to PR: <rust-lang/rust#68528>

💔 book on windows: test-pass → test-fail (cc @carols10cents @steveklabnik).
bors added a commit that referenced this pull request Mar 1, 2020
…s, r=matthewjasper

Skip `Drop` terminators for enum variants without drop glue

Split out from #68528.

When doing drop elaboration for an `enum` that may or may not be moved out of (an open drop), we check the discriminant of the `enum` to see whether the live variant has any drop flags and then check the drop flags to see whether we need to drop each field. Sometimes, however, the live
variant has no move paths and thus no drop flags. In this case, we still emit a drop terminator
for the entire enum after checking the enum discriminant. This drop shim will check the discriminant of the enum *again* and then drop the fields of the active variant. If the active variant has no drop glue, nothing will be done.

This commit skips emitting the drop terminator during drop elaboration when the "otherwise" variants, those without move paths, have no drop glue. A common example of this scenario is when an `Option` is moved from, since `Option::None` never needs drop glue. Below is a fragment the pre-codegen CFG for `Option::unwrap_or` in which we check the drop flag (`_5`) for `self` (`_1`), before and after the change.

Before:

![image](https://user-images.githubusercontent.com/29463364/74078927-52942380-49e5-11ea-8e34-4b9d6d94ef25.png)

After:

![image](https://user-images.githubusercontent.com/29463364/74078945-78b9c380-49e5-11ea-8302-b043c4a7515a.png)

This change doesn't do much on its own, but it is a prerequisite to get the perf gains from #68528.

cc @arielb1
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2020
…=oli-obk

Handle inactive enum variants in `MaybeUninitializedPlaces`

Resolves the first part of rust-lang#69715.

This is the equivalent of rust-lang#68528 but for `MaybeUninitializedPlaces`. Because we now notify drop elaboration that inactive enum variants might be uninitialized, some drops get marked as ["open" that were previously "static"](https://github.com/rust-lang/rust/blob/e0e5d82e1677c82d209b214bbfc2cc5705c2336a/src/librustc_mir/transform/elaborate_drops.rs#L191). Unlike in rust-lang#69715, this isn't strictly better: An "open" drop expands to more MIR than a simple call to the drop shim. However, because drop elaboration considers each field of an "open" drop separately, it can sometimes eliminate unnecessary drops of moved-from or unit-like enum variants. This is the case for `Option::unwrap`, which is reflected in the `mir-opt` test.

cc @eddyb
r? @oli-obk
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2020
…=jonas-schievink

Replace `discriminant_switch_effect` with more general version

rust-lang#68528 added a new edge-specific effect for `SwitchInt` terminators, `discriminant_switch_effect`, to the dataflow framework. While this accomplished the short-term goal of making drop elaboration more precise, it wasn't really useful in other contexts: It only supported `SwitchInt`s on the discriminant of an `enum` and did not allow effects to be applied along the "otherwise" branch. In const-propagation, for example, arbitrary edge-specific effects for the targets of a `SwitchInt` can be used to remember the value a `match` scrutinee must have in each arm.

This PR replaces `discriminant_switch_effect` with a more general `switch_int_edge_effects` method. The new method has a slightly different interface from the other edge-specific effect methods (e.g. `call_return_effect`). This divergence is explained in the new method's documentation, and reading the changes to the various dataflow impls as well as `direction.rs` should further clarify things. This PR should not change behavior.
@ecstatic-morse ecstatic-morse deleted the maybe-init-variants branch October 6, 2020 01:42
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.