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

Modify SimplifyArmIdentity so it can trigger on mir-opt-level=1 #69756

Merged
merged 1 commit into from
May 14, 2020

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Mar 6, 2020

I also added test cases to make sure the optimization can fire on all of
these cases:

fn case_1(o: Option<u8>) -> Option<u8> {
  match o {
    Some(u) => Some(u),
    None => None,
  }
}

fn case2(r: Result<u8, i32>) -> Result<u8, i32> {
  match r {
    Ok(u) => Ok(u),
    Err(i) => Err(i),
  }
}

fn case3(r: Result<u8, i32>) -> Result<u8, i32> {
  let u = r?;
  Ok(u)
}

Without MIR inlining, this still does not completely optimize away the
? operator because the Try::into_result(), From::from() and
Try::from_error() calls still exist. This does move us a bit closer to
that goal though because:

  • We can now run the pass on mir-opt-level=1

  • We no longer depend on the copy propagation pass running which is
    unlikely to stabilize anytime soon.

Fixes #66855

@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Mar 6, 2020

⌛ Trying commit b5238fb with merge 69665ae...

bors added a commit that referenced this pull request Mar 6, 2020
[WIP] Modify SimplifyArmIdentity so it can trigger on mir-opt-level=1

r? @ghost
@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 6, 2020

☀️ Try build successful - checks-azure
Build commit: 69665ae (69665ae05cc719832de28428bece87b264ef69ad)

@rust-timer
Copy link
Collaborator

Queued 69665ae with parent b818ccc, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 69665ae, comparison URL.

@wesleywiser wesleywiser force-pushed the simplify_try branch 2 times, most recently from a77c0f9 to f0d5e44 Compare March 9, 2020 02:38
@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Mar 9, 2020

⌛ Trying commit f0d5e44 with merge be3cc06...

bors added a commit that referenced this pull request Mar 9, 2020
[WIP] Modify SimplifyArmIdentity so it can trigger on mir-opt-level=1

r? @ghost
@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 9, 2020

☀️ Try build successful - checks-azure
Build commit: be3cc06 (be3cc06352a0c63fce9b16c057a58b7f238051ee)

@rust-timer
Copy link
Collaborator

Queued be3cc06 with parent 2cb0b85, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit be3cc06, comparison URL.

@wesleywiser
Copy link
Member Author

I've manually verified the syn-opt benchmark and this optimization doesn't fire at all (the MIR and LLVM-IR before and after this change is the same) so the "regression" is just noise.

@wesleywiser wesleywiser force-pushed the simplify_try branch 2 times, most recently from fa55b55 to 1e0048d Compare March 10, 2020 01:22
@wesleywiser wesleywiser changed the title [WIP] Modify SimplifyArmIdentity so it can trigger on mir-opt-level=1 Modify SimplifyArmIdentity so it can trigger on mir-opt-level=1 Mar 10, 2020
@wesleywiser
Copy link
Member Author

r? @oli-obk

@rust-highfive

This comment has been minimized.

src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/simplify_try.rs Show resolved Hide resolved
@wesleywiser
Copy link
Member Author

@oli-obk The build failure is legit. I've got a repro of the issue with liballoc and I'll include a regression test when I have a fix.

@wesleywiser wesleywiser force-pushed the simplify_try branch 2 times, most recently from 49972c8 to a8189ff Compare March 13, 2020 02:10
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 12, 2020
@bors
Copy link
Contributor

bors commented May 12, 2020

⌛ Testing commit 6de6d70 with merge c892c7cd3cf05930ed4d1fdc7ff025c38dad1969...

@bors
Copy link
Contributor

bors commented May 12, 2020

💔 Test failed - checks-actions

@bors 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 May 12, 2020
@wesleywiser
Copy link
Member Author

@bors retry

Spurious macOS failure.

@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 May 12, 2020
@bors
Copy link
Contributor

bors commented May 12, 2020

⌛ Testing commit 6de6d70 with merge 618120aeea8892e23b8b466bab4c1f308cfaae42...

@Dylan-DPC-zz
Copy link

@bors retry yield

@bors
Copy link
Contributor

bors commented May 13, 2020

⌛ Testing commit 6de6d70 with merge c6b39cc0035a644fe95217a97de42d261a53d7c0...

@bors
Copy link
Contributor

bors commented May 13, 2020

💔 Test failed - checks-actions

@bors 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 May 13, 2020
@oli-obk
Copy link
Contributor

oli-obk commented May 13, 2020

@bors retry artifact upload failure

@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 May 13, 2020
@bors
Copy link
Contributor

bors commented May 14, 2020

⌛ Testing commit 6de6d70 with merge 7c34d8d...

@bors
Copy link
Contributor

bors commented May 14, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 7c34d8d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 14, 2020
@bors bors merged commit 7c34d8d into rust-lang:master May 14, 2020
@nnethercote
Copy link
Contributor

This caused a perf regression for issue-58319. It's not important because that's an artificial microbenchmark that once caused stack overflows and is present just to make sure that doesn't happen again, but I mention it for completeness.

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2020

Curious that the regression didn't show up in the perf runs in this PR. Maybe some other change snuck in between that caused this change to become a perf regression?

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2020

Also, according to https://perf.rust-lang.org/detailed-query.html?commit=7c34d8d6629506a596215886e5fc4bb2b04b00ae&base_commit=23ffeea307c31f0c20ebb5a15d5171e0c414629d&benchmark=issue-58319-check&run_name=full the regression is entirely inside borrowck, which isn't affected by this change beyond this change affecting the built rustc. So... did we make rustc slower (I honestly fail to see how)

@wesleywiser
Copy link
Member Author

Total time for the issue-58319 test is in the hundredths of seconds. That may just be too small a runtime for the self-profile time column to be comparable. For the mir_borrowck query, the variance is 1/1000th of a second.

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.

[MIR-opt] trivial matches aren't optimized out at mir-opt-level=1