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

Fix unsound optimization with explicit variant discriminants #89489

Merged
merged 3 commits into from
Oct 4, 2021
Merged

Fix unsound optimization with explicit variant discriminants #89489

merged 3 commits into from
Oct 4, 2021

Conversation

FabianWolff
Copy link
Contributor

@FabianWolff FabianWolff commented Oct 3, 2021 •
edited
Loading

Fixes #89485.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 3, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 3, 2021

lgtm, tho we should do some cleanups of the opt after this is merged

r? @tmiasko for another pair of eyes

@tmiasko
Copy link
Contributor

tmiasko commented Oct 3, 2021

The change seems reasonable.

At the same time I can't really follow the preexisting implementation. Why the left hand side of the assignment is ignored for the purposes of the comparison? How do we know that place that we read the discriminant from still holds the same value?

@camelid camelid added the A-mir-opt Area: MIR optimizations label Oct 3, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 3, 2021 •
edited
Loading

@tmiasko so I guess let's disable the opt in addition to this fix until we have a clearer picture of how it works?

@tmiasko
Copy link
Contributor

tmiasko commented Oct 3, 2021

let's disable the opt in addition to this fix until we have a clearer picture of how it works?

Sounds good to me.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 3, 2021

@FabianWolff please also add an early abort in the mir opt with

if !tcx.sess.opts.debugging_opts.unsound_mir_opts {
    return;
}

so that we don't run the opt at all in normal mode

@FabianWolff
Copy link
Contributor Author

@oli-obk Done.

@wesleywiser
Copy link
Member

@bors r=oli-obk p=1

Fixes P-critical issue.

@bors
Copy link
Contributor

bors commented Oct 3, 2021

📌 Commit dd9b476 has been approved by oli-obk

@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 Oct 3, 2021
@wesleywiser
Copy link
Member

wesleywiser commented Oct 3, 2021 •
edited
Loading

Beta nominating so we can get this fix out sooner. The most important change is that we've disabled the pass by default which is safe and should be very easy to backport.

@wesleywiser wesleywiser added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 3, 2021
@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 3, 2021
@bors
Copy link
Contributor

bors commented Oct 4, 2021

⌛ Testing commit dd9b476 with merge a6bea3af357e9103d5a9470030a30980f63deed3...

@nbdd0121
Copy link
Contributor

nbdd0121 commented Oct 4, 2021

Should this be backported to stable?

Aaron1011
Aaron1011 reviewed Oct 4, 2021
View reviewed changes
src/test/ui/mir/issue-89485.rs
@@ -0,0 +1,18 @@
// Regression test for issue #89485.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be a good idea to add a copy of this test with -Zunsound-mir-opts enabled

@bors
Copy link
Contributor

bors commented Oct 4, 2021

💥 Test timed out

@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 Oct 4, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@wesleywiser
Copy link
Member

@bors retry

@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 Oct 4, 2021
@bors
Copy link
Contributor

bors commented Oct 4, 2021

⌛ Testing commit dd9b476 with merge a479766...

@bors
Copy link
Contributor

bors commented Oct 4, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing a479766 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2021
@bors bors merged commit a479766 into rust-lang:master Oct 4, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 4, 2021
@wesleywiser
Copy link
Member

Should this be backported to stable?

It's a good question @nbdd0121 . IMO, since we're about 1.5 weeks from the next release, beta backport seems sufficient to me.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a479766): comparison url.

Summary: This change led to small relevant regressions 😿 in compiler performance.

  • Small regression in instruction counts (up to 2.3% on full builds of style-servo)

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 Oct 4, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 4, 2021

@rustbot label: +perf-regression-triaged

we disabled an unsound MIR opt. LLVM can't figure this out on its own sadly.

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 4, 2021
@apiraino
Copy link
Contributor

apiraino commented Oct 7, 2021

Beta backport accepted as per compiler team on Zulip

Note from the meeting: "if there’s any issue at all, its also fine to just backport solely the bit that disables the pass"

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 7, 2021
@cuviper cuviper mentioned this pull request Oct 13, 2021
Merged
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 13, 2021
@cuviper cuviper modified the milestones: 1.57.0, 1.56.0 Oct 13, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2021
@bors
Auto merge of rust-lang#89854 - cuviper:beta-next, r=Mark-Simulacrum
58268ff 
[beta] backports

- 2229: Consume IfLet expr rust-lang#89282
- Wrapper for -Z gcc-ld=lld to invoke rust-lld with the correct flavor rust-lang#89288
- Fix unsound optimization with explicit variant discriminants rust-lang#89489
- Fix stabilization version for bindings_after_at rust-lang#89605
- Turn vtable_allocation() into a query rust-lang#89619
- Revert "Stabilize Iterator::intersperse()" rust-lang#89638
- Ignore type of projections for upvar capturing rust-lang#89648
- ~~Add Poll::ready and~~ revert stabilization of task::ready! rust-lang#89651
- CI: Use mirror for libisl downloads for more docker dist builds rust-lang#89661
-  Use correct edition for panic in [debug_]assert!(). rust-lang#89622
-  Switch to our own mirror of libisl plus ct-ng oldconfig fixes rust-lang#89599
-  Emit item no type error even if type inference fails rust-lang#89585
-  Revert enum discriminants rust-lang#89884
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@Aaron1011 Aaron1011 Aaron1011 left review comments

Assignees

@oli-obk oli-obk

Labels
A-mir-opt Area: MIR optimizations 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.56.0
Development

Successfully merging this pull request may close these issues.

SimplifyBranchSame assumes discriminant is the same as variant index.