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 MatchBranchSimplification #78151

Merged
merged 1 commit into from
Oct 20, 2020
Merged

Disable MatchBranchSimplification #78151

merged 1 commit into from
Oct 20, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 20, 2020 •
edited
Loading

This optimization can result in unsoundness, because it introduces
additional uses of a place holding the discriminant value without
ensuring that it is valid to do so.

Found by validation from #77369 / #78147.

@tmiasko
Disable MatchBranchSimplification
c2af254 
This optimization can result in unsoundness, because it introduces
additional uses of a place holding the discriminant value without
ensuring that it is valid to do so.
@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 20, 2020
@jonas-schievink jonas-schievink added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 20, 2020
@jonas-schievink
Copy link
Contributor

The pass was introduced in #75382, so nominating for beta+stable backport

@jonas-schievink jonas-schievink added stable-nominated Nominated for backporting to the compiler in the stable channel. A-mir-opt Area: MIR optimizations I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Oct 20, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 20, 2020
@LeSeulArtichaut LeSeulArtichaut removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 20, 2020
@jonas-schievink
Copy link
Contributor

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Oct 20, 2020

📌 Commit 478d5ef70a586653674eaf25df2e4a1da5628567 has been approved by jonas-schievink

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

bors commented Oct 20, 2020

⌛ Testing commit 478d5ef70a586653674eaf25df2e4a1da5628567 with merge 28ffbc3a0e1b47860f881aa91fb7755ebb538eb0...

@wesleywiser
Copy link
Member

I think the 64-bit tests need to be blessed as well. (I only see a change to src/test/mir-opt/matches_reduce_branches.bar.MatchBranchSimplification.32bit.diff)

@tmiasko tmiasko force-pushed the disable-match-branch-simplification branch from 478d5ef to c2af254 Compare October 20, 2020 16:55
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 20, 2020

Updated 64-bit test output as well.

@wesleywiser
Copy link
Member

I'm not sure if we need to wait for bors to fail before re-approving, but I'll go ahead and approve in case that works.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2020

📌 Commit c2af254 has been approved by wesleywiser

@bors
Copy link
Contributor

bors commented Oct 20, 2020

⌛ Testing commit c2af254 with merge 981346f...

@bors bors mentioned this pull request Oct 20, 2020
Merged
@bors
Copy link
Contributor

bors commented Oct 20, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: wesleywiser
Pushing 981346f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 20, 2020
@bors bors merged commit 981346f into rust-lang:master Oct 20, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 20, 2020
@tmiasko tmiasko deleted the disable-match-branch-simplification branch October 20, 2020 19:26
@apiraino apiraino mentioned this pull request Oct 21, 2020
Open
@Mark-Simulacrum
Copy link
Member

Seems to have been a win on a number of benchmarks, https://perf.rust-lang.org/compare.html?start=9832374f6e378971e1a933362cf9781b121bb845&end=981346fc07dd5ef414c5b1b21999f7604cece006&stat=instructions:u. Good work!

@spastorino
Copy link
Member

discussed in T-compiler meeting.

@rustbot modify labels: beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 22, 2020
@spastorino
Copy link
Member

discussed in T-compiler meeting.

@rustbot modify labels: stable-accepted

@rustbot rustbot added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Oct 22, 2020
@pnkfelix pnkfelix mentioned this pull request Oct 22, 2020
Closed
@tmiasko tmiasko mentioned this pull request Oct 22, 2020
Merged
@ErichDonGubler
Copy link
Contributor

Might this issue be included in a point release, or is the problem this is fixing not terribly risky? I'm attempting to triage potential issues for upgrading to Rust 1.47, and this issue has been identified as the only major question we're aware of right now for that release.

@Mark-Simulacrum
Copy link
Member

My understanding that it is not terribly risky. This should be included in 1.48, which will be released in less than 3 weeks (on the 19th).

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 5, 2020
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.49.0, 1.48.0 Nov 5, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Nov 5, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 7, 2020
@bors
Auto merge of rust-lang#78783 - Mark-Simulacrum:beta-next, r=Mark-Sim…
4f9dc0a 
…ulacrum

[beta] backports

This backports a number of PRs to beta:

* Add delay_span_bug to no longer ICE rust-lang#78645
* Do not ICE on invalid input rust-lang#78422
* revert rust-lang#75443, update mir validator rust-lang#78410
* Do not try to report on closures to avoid ICE rust-lang#78268
* Disable "optimization to avoid load of address" in InstCombine rust-lang#78195
* Disable MatchBranchSimplification rust-lang#78151
* Do not ICE with TraitPredicates containing [type error] rust-lang#77930
* Tweak `if let` suggestion to be more liberal with suggestion and to not ICE rust-lang#77283
* Use different mirror for linux headers in musl-toolchain CI script. rust-lang#78316
@Mark-Simulacrum Mark-Simulacrum removed stable-accepted Accepted for backporting to the compiler in the stable channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
A-mir-opt Area: MIR optimizations beta-accepted Accepted for backporting to the compiler in the beta channel. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.