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

Unreachable propagation: preserve information about unreachable values #77800

Closed
wants to merge 4 commits into from

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 10, 2020

Unreachable propagation pass removes all unreachable terminators.
As a consequence, for switch int terminators, where two or more targets
remain after the optimization, where otherwise branch is reachable,
switching on unreachable values will use otherwise branch after the
optimization. Consequently, the information about unreachable values is
irrevocably lost.

To preserve the information about unreachable values, keep the existing
terminator if otherwise branch is reachable. Additionally, since now
unreachable blocks might remain in use, remove their statements.

Based on analysis from #77680, also enables unreachable propagation by default.

jonas-schievink and others added 4 commits October 10, 2020 19:45
Unreachable propagation pass removes all unreachable terminators.
As a consequence, for switch int terminators, where two or more targets
remain after the optimization, where otherwise branch is reachable,
switching on unreachable values will use otherwise branch after the
optimization. Consequently, the information about unreachable values is
irrevocably lost.

To preserve the information about unreachable values, keep the existing
terminator if otherwise branch is reachable. Additionally, since now
unreachable blocks might remain in use, remove their statements.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 10, 2020

⌛ Trying commit 1ad305e with merge 48e255ae10a076b047a4ee7b01befa7cdde97d38...

@bors
Copy link
Contributor

bors commented Oct 10, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 48e255ae10a076b047a4ee7b01befa7cdde97d38 (48e255ae10a076b047a4ee7b01befa7cdde97d38)

@rust-timer
Copy link
Collaborator

Queued 48e255ae10a076b047a4ee7b01befa7cdde97d38 with parent 87b71ed, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (48e255ae10a076b047a4ee7b01befa7cdde97d38): 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

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 11, 2020

No impact on compilation time.

We should teach unreachable propagator about unreachable intrinsic or change how MIR is built.

@tmiasko tmiasko closed this Oct 11, 2020
@tmiasko tmiasko deleted the unreachable-or-not branch October 11, 2020 09:16
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.

5 participants