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

Turn on UnreachablePropagation by default #77680

Closed
wants to merge 2 commits into from
Closed

Turn on UnreachablePropagation by default #77680

wants to merge 2 commits into from

Conversation

jonas-schievink
Copy link
Contributor

This was added in #66329, but turned off by default since it ran into pathological LLVM behavior leading to huge compile time regressions (#68105). Let's see if that is still the case.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 7, 2020
@jonas-schievink
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 Oct 7, 2020

⌛ Trying commit a9271aa28d57e04ba28b891131eaa8cabedc16fa with merge 79c78c83b88c8bad28f42ba333ed1b59984db9af...

@jonas-schievink
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 Oct 7, 2020

⌛ Trying commit 8ca4f65 with merge 9def798b7065dbe3fba4426c64a637b6c1eeb6cd...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-linux 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.
   Compiling rustc_mir v0.0.0 (/checkout/compiler/rustc_mir)
error: unused variable: `tcx`
  --> compiler/rustc_mir/src/transform/unreachable_prop.rs:15:30
   |
15 |     fn run_pass<'tcx>(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
   |                              ^^^ help: if this is intentional, prefix it with an underscore: `_tcx`
   |
   = note: `-D unused-variables` implied by `-D warnings`
error: aborting due to previous error

error: could not compile `rustc_mir`.


To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "jemalloc llvm max_level_info" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host x86_64-unknown-linux-gnu --target x86_64-unknown-linux-gnu
Build completed unsuccessfully in 0:12:13
== clock drift check ==
  local time: Wed Oct  7 23:25:53 UTC 2020
  local time: Wed Oct  7 23:25:53 UTC 2020
  network time: Wed, 07 Oct 2020 23:25:53 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (4408) (python)

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 @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Oct 7, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 9def798b7065dbe3fba4426c64a637b6c1eeb6cd (9def798b7065dbe3fba4426c64a637b6c1eeb6cd)

@rust-timer
Copy link
Collaborator

Queued 9def798b7065dbe3fba4426c64a637b6c1eeb6cd with parent 4437b4b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (9def798b7065dbe3fba4426c64a637b6c1eeb6cd): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@jonas-schievink
Copy link
Contributor Author

It looks like this causes some minor runtime perf regressions in rustc (mir_borrowck, typeck and check_mod_privacy silghtly regressed), but does generally improve compile times.

The regressions might simply be due to different inlining decisions / code partitioning, but I haven't looked into them in detail.

@jonas-schievink
Copy link
Contributor Author

Blessed the mir-opt tests, but now some codegen tests are failing (eg. codegen/match-optimizes-away.rs).

This seems to make sense, because LLVM now has less info to work with since the unreachable block often isn't present anymore (so for example it loses range information for integers that are switched on). This also explains the perf regression.

@jonas-schievink
Copy link
Contributor Author

cc #77800

Closing for now, until we figure out what to do with optimizations that improve compile times but regress runtime performance

Noratrieb added a commit to Noratrieb/rust that referenced this pull request Aug 21, 2022
It was disabled because of pathological behaviour of LLVM in some
benchmarks. As of rust-lang#77680, this has been fixed. The problem there was
that it caused pessimizations in some cases. These have now been fixed
as well.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2022
UnreachableProp: Preserve unreachable branches for multiple targets

Before, UnreachablePropagation removed all unreachable branches. This was a pessimization, as it removed information about reachability that was used later in the optimization pipeline.

For example, this code
```rust
pub enum Two { A, B }
pub fn identity(x: Two) -> Two {
    match x {
        Two::A => Two::A,
        Two::B => Two::B,
    }
}
```

basically has `switchInt() -> [0: 0, 1: 1, otherwise: unreachable]` for the match. This allows it to be transformed into a simple `x`. If we remove the unreachable branch, the transformation becomes illegal.

This was the problem keeping `UnreachablePropagation` from being enabled, so we can enable it now.

Something similar already happened in rust-lang#77800, but it did not show a perf improvement there. Let's try it again anyways!

Fixes rust-lang#68105, although that issue has been fixed for a long time (see rust-lang#77680).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants