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 the SimplifyArmIdentity pass #73262

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Disable the SimplifyArmIdentity pass #73262

merged 1 commit into from
Jun 12, 2020

Conversation

wesleywiser
Copy link
Member

This pass is buggy so I'm disabling it to fix a stable-to-beta
regression.

Related to #73223

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2020

If we aren't in a rush about this, please open a PR against master with this change that we can backport to beta.

@wesleywiser
Disable the SimplifyArmIdentity pass on beta
f160583 
This pass is buggy so I'm disabling it to fix a stable-to-beta
regression.

Related to rust-lang#73223
@wesleywiser wesleywiser changed the base branch from beta to master June 12, 2020 12:42
@bors
Copy link
Contributor

bors commented Jun 12, 2020

☔ The latest upstream changes made this pull request unmergeable. Please resolve the merge conflicts.

@wesleywiser wesleywiser force-pushed the simplifyarmidentity_beta_regression branch from 5f63f6d to f160583 Compare June 12, 2020 12:43
@wesleywiser
Copy link
Member Author

Rebased

@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 12, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jun 12, 2020

📌 Commit f160583 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 Jun 12, 2020
@wesleywiser
Copy link
Member Author

Since the linked PR is P-critical, I'm raising the priority of this.

@bors p=1

@Mark-Simulacrum Mark-Simulacrum changed the title Disable the SimplifyArmIdentity pass on beta Disable the SimplifyArmIdentity pass Jun 12, 2020
@Dylan-DPC-zz
Copy link

bumping this higher to run after the current rollup

@bors p=3

@bors
Copy link
Contributor

bors commented Jun 12, 2020

⌛ Testing commit f160583 with merge 664f6df82aa3cc4f3ad1eec7dd277b061938f7da...

@bors
Copy link
Contributor

bors commented Jun 12, 2020

💔 Test failed - checks-azure

@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 Jun 12, 2020
@Dylan-DPC-zz
Copy link

@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 Jun 12, 2020
@Dylan-DPC-zz
Copy link

@bors retry

@Dylan-DPC-zz
Copy link

@bors retru p=1000

@Dylan-DPC-zz
Copy link

@bors retry p=1000

@bors
Copy link
Contributor

bors commented Jun 12, 2020

⌛ Testing commit f160583 with merge f4fbb93...

@Dylan-DPC-zz
Copy link

@bors treeclosed-

@Dylan-DPC-zz
Copy link

@bors treeclosed--

@bors
Copy link
Contributor

bors commented Jun 12, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 12, 2020
@bors bors merged commit f4fbb93 into rust-lang:master Jun 12, 2020
@nnethercote
Copy link
Contributor

This caused a small perf regression in a couple of opt benchmarks.

@wesleywiser: will this pass be re-enabled in the future?

@wesleywiser
Copy link
Member Author

@nnethercote Yes! I'm working on finishing up some changes that will re-enable this on nightly.

@spastorino spastorino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 17, 2020
@pnkfelix
Copy link
Member

The manner in which this was disabled seems relevant to a recent wg-mir-opt conversation on zulip.

I guess if opt-level >= 2 is generally known to denote potentially buggy optimizations, then what's done here is okay.

But I don't know if that's the consensus based on the above linked conversation...

@pnkfelix
Copy link
Member

(I was tempted to unilaterally beta approve this, but I'm hesitating due to not knowing if it matches the semantics everyone else expects from the mir-opt levels. We'll just discuss as a group tomorrow instead.)

@wesleywiser
Copy link
Member Author

@pnkfelix The current state is that mir-opt-level >= 2 has buggy optimizations. There isn't too much of a semantic difference between 2 or 3 currently although I am in favor of moving to a scheme where mir-opt-level=0 is no optimizations, mir-opt-level=1 is stable optimizations, mir-opt-level=2 is unstable but nearly correct optimizations (or perhaps just optimizations that hurt compile-time), mir-opt-level=3 is code we want to keep from bit-rotting but has issues or something similar.

@nikomatsakis
Copy link
Contributor

@wesleywiser I presume you'll add some tests that demonstrate the bug, too?

@wesleywiser
Copy link
Member Author

@nikomatsakis Yes, I'm adding some regression tests in the next PR.

@pnkfelix
Copy link
Member

Discussed at T-compiler meeting; approved for beta backport

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 18, 2020
RalfJung
RalfJung reviewed Jun 22, 2020
View reviewed changes
src/librustc_mir/transform/simplify_try.rs
@@ -306,7 +306,11 @@ fn optimization_applies<'tcx>(
}

impl<'tcx> MirPass<'tcx> for SimplifyArmIdentity {
fn run_pass(&self, _: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) {
fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level < 2 {
Copy link
Member

Choose a reason for hiding this comment

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

Uh, could we maybe not keep buggy passes running at all? I think a -Zmir-opt-level=3 miscompilation is a soundness bug just like it would be at a lower optimization level.

C/C++ programmers seem to associate "high optimization levels" with "creates buggy code", but really that's awful and we shouldn't do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@RalfJung I opened a related MCP: rust-lang/compiler-team#319

If you have thoughts about it, please leave a comment there. Thanks!

@RalfJung RalfJung mentioned this pull request Jun 25, 2020
Closed
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 26, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jun 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2020
@bors
Auto merge of rust-lang#73766 - Mark-Simulacrum:beta-next, r=Mark-Sim…
8196407 
…ulacrum

[beta] backports

This PR backports the following:

* Make novel structural match violations not a `bug` rust-lang#73446
* linker: Never pass `-no-pie` to non-gnu linkers rust-lang#73384
* Disable the `SimplifyArmIdentity` pass rust-lang#73262
* Allow inference regions when relating consts rust-lang#73225
* Fix link error with #[thread_local] introduced by rust-lang#71192 rust-lang#73065
* Ensure stack when building MIR for matches rust-lang#72941
* Don't create impl candidates when obligation contains errors rust-lang#73005

r? @ghost
@wesleywiser wesleywiser mentioned this pull request Oct 9, 2020
Closed
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@eddyb eddyb

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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.46
Development

Successfully merging this pull request may close these issues.