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 Broken MIR on match branch simplification w/ u8 #75507 #75508

Closed
wants to merge 2 commits into from
Closed

Fix Broken MIR on match branch simplification w/ u8 #75507 #75508

wants to merge 2 commits into from

Conversation

JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Aug 13, 2020 •
edited
Loading

Fixes #75507 yet, and this adds a regression test.

Done by explicitly checking that the type of the const is a bool.

r? @oli-obk cc: @matthiaskrgr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2020
@JulianKnodt
Copy link
Contributor Author

I checked and this only breaks on u8, which might possibly be because u8 and bool have the same representation.

@JulianKnodt JulianKnodt force-pushed the opt_bug branch 2 times, most recently from bfb4da5 to 670fa77 Compare August 14, 2020 00:29
@JulianKnodt JulianKnodt changed the title Add regression test for matching on u8 Fix Broken MIR on match branch simplification w/ u8 #75507 Aug 14, 2020
@JulianKnodt JulianKnodt force-pushed the opt_bug branch 2 times, most recently from c6ec708 to 5b1481f Compare August 14, 2020 01:01
@JulianKnodt
Add fix
fd74026 
This also explicitly checks that the types are `bool`. `try_eval_bool` also appears to just
succeed for `u8`, so this ensures that it actually is a bool before casting.
oli-obk
oli-obk reviewed Aug 14, 2020
View reviewed changes
src/test/mir-opt/matches_u8.rs
@@ -0,0 +1,32 @@
// EMIT_MIR_FOR_EACH_BIT_WIDTH
Copy link
Contributor

Choose a reason for hiding this comment

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

Are our mir-opt tests running on mir-opt-level 3 by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, I'm not sure, but when I ran it initially with this file it failed

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 they run with mir-opt-level=2 but I can't find it in the source off hand.

Copy link
Contributor

Choose a reason for hiding this comment

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

rust/src/tools/compiletest/src/runtest.rs

Lines 1941 to 1946 in 8e21bd0

MirOpt => {
rustc.args(&[
"-Zdump-mir=all",
"-Zmir-opt-level=3",
"-Zdump-mir-exclude-pass-number",
]);

@tmiasko tmiasko mentioned this pull request Aug 14, 2020
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Aug 15, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 15, 2020

📌 Commit fd74026 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 Aug 15, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 15, 2020

@bors r-

closing in favour of the other PR that's based on this one

@oli-obk oli-obk closed this Aug 15, 2020
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 15, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2020
@bors
Auto merge of rust-lang#75537 - tmiasko:match-branch-simplify, r=oli-obk
b9db927 
MatchBranchSimplification: fix equal const bool assignments

The match branch simplification is applied when target blocks contain
statements that are either equal or perform a const bool assignment with
different values to the same place.

Previously, when constructing new statements, only statements from a
single block had been examined. This lead to a misoptimization when
statements are equal because the assign the *same* const bool value to
the same place.

Fix the issue by examining statements from both blocks when deciding on
replacement.

Additionally:

* Copy discriminant instead of moving it since it might be necessary to use its
  value more than once.
* Optimize when switching on copy operand

Based on rust-lang#75508.

r? @oli-obk  / @JulianKnodt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@wesleywiser wesleywiser wesleywiser left review comments

@tmiasko tmiasko tmiasko left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LLVM_ERROR with -Zmir-opt-level=3: match.rs
6 participants
@JulianKnodt @oli-obk @bors @wesleywiser @tmiasko @rust-highfive