Skip to content

Fix change detection in CfgSimplifier::collapse_goto_chain #75076

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

Merged
merged 2 commits into from
Aug 3, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Aug 2, 2020

Check that the old target is different from the new collapsed one, before concluding that anything changed.

Fixes #75074
Fixes #75051

tmiasko added 2 commits August 3, 2020 00:39
Check that the old target is different from the new collapsed one,
before concluding that anything changed.
@rust-highfive
Copy link
Contributor

r? @lcnr

(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 Aug 2, 2020
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 2, 2020

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned lcnr Aug 2, 2020
@@ -212,6 +210,7 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
Terminator { kind: TerminatorKind::Goto { ref mut target }, .. } => target,
_ => unreachable!(),
};
*changed |= *target != last;
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ecstatic-morse We now have a test case for this!

@@ -0,0 +1,7 @@
// Caused an infinite loop during SimlifyCfg MIR transform previously.
//
// build-pass
Copy link
Contributor

Choose a reason for hiding this comment

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

does this really test this mir pass? don't you also need -Zmir-opt-level=2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this test case no additional flags are necessary to reproduce the issue. Unlike the one from #75051 which also needed -Zmir-opt-level=2.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, superb! thanks

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 3, 2020

📌 Commit 7f9f2ff 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 3, 2020
@bors
Copy link
Collaborator

bors commented Aug 3, 2020

⌛ Testing commit 7f9f2ff with merge dbc2ef2...

@bors
Copy link
Collaborator

bors commented Aug 3, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2020
@bors bors merged commit dbc2ef2 into rust-lang:master Aug 3, 2020
@tmiasko tmiasko deleted the simplify-goto branch August 3, 2020 09:07
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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
6 participants