[MIR-OPT]: Optimization that turns Eq-Not pair into Ne #77031

simonvandel · 2020-09-21T21:42:50Z

No description provided.

rust-highfive · 2020-09-21T21:42:53Z

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

simonvandel · 2020-09-21T21:45:52Z

src/test/mir-opt/issue_73223.main.SimplifyArmIdentity.64bit.diff

          StorageDead(_16);                // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
-          switchInt(_15) -> [false: bb4, otherwise: bb5]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
+          switchInt(move _17) -> [1_i32: bb4, otherwise: bb5]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL


I think this is a pretty cool interaction between SimplifyComparisonIntegral and this optimization that makes it possible to eliminate the Ne entirely, since we can just switch on _17 directly.

oli-obk · 2020-09-21T21:55:47Z

compiler/rustc_mir/src/transform/instcombine.rs

+                        if *l == place {
+                            match r {
+                                Rvalue::BinaryOp(BinOp::Eq, op1, op2) => {
+                                    // We need to make sure that the StorageDeads we saw are for


I don't understand why this is important. I'd rather think that the StorageDead for l is important, otherwise we should not remove the Neg, but instead also copy the result of the Ne operation to l.

Can you expand on your statement? I don't understand your point about not removing Neq (you mean Not? or Ne?).

Now that I think about it, yeah, it is perhaps unneccessary to require that the StorageDeads are for the Eq operands. Hmm do we even need to do anything special about potential StorageDeads between Eq and Not? I think not, besides doing the swap, which I think we can just do unconditionally for simplicity. We already assert that only StorageDead statements can appear between Eq and Not, and I think simply swapping a StorageDead for a complete unrelated local with Ne should not matter.

What I'm wondering is whether it is generally simpler to replace

// _4 = Eq(move _5, move _6); // StorageDead(_5); // StorageDead(_6); // _3 = Not(move _4);

with

// _4 = Ne(move _5, move _6); // StorageDead(_5); // StorageDead(_6); // _3 = move _4;

since then you don't have to care about any liveness or similar and leave further changes to other optimizations.

If you want to get rid of the additional _3 = move 4;, maybe another peephole optimization may be better? One that just checks for _x = foo; /*arbitrary statements not touching _x or _y*/ _y = _x; and changes it to _y = foo; /*arbitrary statements not touching _x or _y*/ nop;

Yeah that might be simpler to do. For simplicity I'll let other passes optimize the move

I think it may be beneficial to keep the current version. With your proposal, the interaction in #77031 (comment) would break. This is only one case, yeah, but it still seems worth to keep that.

What do you think?

I personally would prefer to break that interaction and fix it in a separate PR. Otherwise we need to implement both the simpler version of this PR together with the peephole opt for trivial copy propagation.

compiler/rustc_mir/src/transform/instcombine.rs

oli-obk · 2020-09-21T21:57:29Z

compiler/rustc_mir/src/transform/instcombine.rs

+                                    }
+
+                                    // We need to make sure that the Ne we are going to insert comes before the
+                                    // StorageDeads so we want to swap the StorageDead closest to Eq with Ne.


I thought we are replacing the comparison, not the StorageDead?

I think the confusion comes from the use of "insert"? We are replacing the Not with a Ne. And since we are going to use the operands from Eq in Ne, we need to make sure that we do not mark these operands as dead before they are used. I'll push up a new commit where I try to explain this better.

bors · 2020-09-22T00:50:19Z

☔ The latest upstream changes (presumably #76683) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

wesleywiser · 2020-09-22T13:23:44Z

compiler/rustc_mir/src/transform/instcombine.rs

+    op1: Operand<'tcx>,
+    op2: Operand<'tcx>,


I think it would good to add some doc comments explaining what these fields are for. These two in particular are fairly opaque names but it would be good to have comments for the other fields too.

simonvandel · 2020-09-23T17:03:56Z

Hi @oli-obk and @wesleywiser
I have added a set of commits on top of the original 2, which should resolve your comments.
@oli-obk I still need to understand your one comment.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-09-25T22:10:18Z

☔ The latest upstream changes (presumably #77198) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Dylan-DPC-zz · 2020-10-16T13:20:19Z

@simonvandel if you can merge the conflicts we can get this reviewed

crlf0710 · 2020-11-06T16:25:58Z

@simonvandel Ping from triage, could you resolve the merge conflicts? Also the CI is still red.

JohnCSimon · 2020-11-24T02:04:09Z

@simonvandel
Thanks for your contribution, but unfortunately I'll have to close this due to being inactive for two months, please feel free to reopen when you ready to proceed.

rust-highfive assigned matthewjasper Sep 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2020

simonvandel commented Sep 21, 2020

View reviewed changes

oli-obk reviewed Sep 21, 2020

View reviewed changes

compiler/rustc_mir/src/transform/instcombine.rs Show resolved Hide resolved

oli-obk reviewed Sep 21, 2020

View reviewed changes

wesleywiser reviewed Sep 22, 2020

View reviewed changes

simonvandel added 5 commits September 23, 2020 18:29

add test that does not currently optimize

6e19e8f

Implement optimization that turns Eq-Not pair into Ne

04bb561

Add fixme for future work on Ne-Not pair

df05775

Expand on why we swap StorageDead

65389df

Document EqNotOptInfo fields

0e0e767

simonvandel force-pushed the eq-not branch from 208a69b to 0e0e767 Compare September 23, 2020 17:02

camelid added A-mir-opt Area: MIR optimizations 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2020

crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2020

JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 24, 2020

JohnCSimon closed this Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIR-OPT]: Optimization that turns Eq-Not pair into Ne #77031

[MIR-OPT]: Optimization that turns Eq-Not pair into Ne #77031

simonvandel commented Sep 21, 2020

rust-highfive commented Sep 21, 2020

simonvandel Sep 21, 2020

oli-obk Sep 21, 2020

simonvandel Sep 23, 2020

oli-obk Sep 23, 2020

oli-obk Sep 23, 2020

simonvandel Sep 23, 2020

simonvandel Sep 23, 2020

oli-obk Sep 24, 2020

oli-obk Sep 21, 2020

simonvandel Sep 23, 2020

bors commented Sep 22, 2020

wesleywiser Sep 22, 2020

simonvandel commented Sep 23, 2020

bors commented Sep 25, 2020

Dylan-DPC-zz commented Oct 16, 2020

crlf0710 commented Nov 6, 2020

JohnCSimon commented Nov 24, 2020

[MIR-OPT]: Optimization that turns Eq-Not pair into Ne #77031

[MIR-OPT]: Optimization that turns Eq-Not pair into Ne #77031

Conversation

simonvandel commented Sep 21, 2020

rust-highfive commented Sep 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 22, 2020

Choose a reason for hiding this comment

simonvandel commented Sep 23, 2020

bors commented Sep 25, 2020

Dylan-DPC-zz commented Oct 16, 2020

crlf0710 commented Nov 6, 2020

JohnCSimon commented Nov 24, 2020