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.

Sign up for GitHub

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

Jump to bottom

Disable almost certainly unsound early otherwise branch MIR opt #95161

Merged
merged 1 commit into from Mar 22, 2022
Merged

Disable almost certainly unsound early otherwise branch MIR opt #95161

merged 1 commit into from Mar 22, 2022

Conversation

JakobDegen
Copy link
Contributor

@JakobDegen JakobDegen commented Mar 21, 2022 •
edited

Originally thought this was just an MIR semantics issue, but it's not.

r? rust-lang/mir-opt

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 21, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2022
@JakobDegen JakobDegen changed the title Add note about assumption around MIR semantics to early otherwise branch pass Disable almost certainly unsound early otherwise branch MIR opt Mar 21, 2022
@JakobDegen JakobDegen mentioned this pull request Mar 21, 2022
Closed
bjorn3
bjorn3 reviewed Mar 21, 2022
View reviewed changes
compiler/rustc_mir_transform/src/early_otherwise_branch.rs
// // code
// }
// _ => {
// // don't use Q
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does use Q. The match does tmp = discriminant(Q) and then switchInt(tmp) to jump to the right arm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not actually important what happens in this branch because we don't take the arm of the outer match that includes match q anyway (in the code path we miscompile). It's only important that this branch is trivially the same as the other "don't use q" branch - the optimization will see that they do the same thing, and replace the outer match with

if discrim(p) == discrim(q)

I suppose this should really read "do the same thing as in the other fall through branch"

@wesleywiser
Copy link
Member

wesleywiser commented Mar 21, 2022 •
edited

@bors r+

Beta nominating because the pass rewrite is currently in beta (#91840) and if there are soundness concerns, we should disable there as well.

cc @oli-obk

@bors
Copy link
Contributor

bors commented Mar 21, 2022

📌 Commit a2f3a17 has been approved by wesleywiser

@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 Mar 21, 2022
@wesleywiser
Copy link
Member

There was a perf impact when #91840 merged.

@bors rollup=never

@wesleywiser wesleywiser added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 21, 2022
@bors
Copy link
Contributor

bors commented Mar 22, 2022

⌛ Testing commit a2f3a17 with merge cf85310...

@bors
Copy link
Contributor

bors commented Mar 22, 2022

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing cf85310 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 22, 2022
@bors bors merged commit cf85310 into rust-lang:master Mar 22, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 22, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cf85310): comparison url.

Summary: This benchmark run shows 16 relevant regressions 😿 to instruction counts.

  • Arithmetic mean of relevant regressions: 3.1%
  • Largest regression in instruction counts: 8.5% on full builds of deeply-nested-multi check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Mar 22, 2022
@Mark-Simulacrum
Copy link
Member

@rustbot label +perf-regression-triaged

This is a soundness fix, and impact on primary benchmarks is minimal. Marking as triaged.

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 22, 2022
@JakobDegen JakobDegen deleted the fix-early-otherwise-branch branch April 11, 2022 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@JakobDegen @wesleywiser @bors @rust-timer @Mark-Simulacrum @bjorn3 @rust-highfive @rustbot