Skip to content

Canonicalize operands for commutative binary ops - #163373

Merged
rust-bors[bot] merged 2 commits into
rust-lang:mainfrom
mu001999-contrib:fix-162220
Oct 17, 2026
Merged

Canonicalize operands for commutative binary ops#163373
rust-bors[bot] merged 2 commits into
rust-lang:mainfrom
mu001999-contrib:fix-162220

Conversation

@mu001999

@mu001999 mu001999 commented Sep 26, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

@rustbot

rustbot commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 26, 2026
@rustbot

rustbot commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 77 candidates
  • Random selection from 18 candidates

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
jieyouxu
jieyouxu reviewed Sep 26, 2026
View reviewed changes
Comment thread compiler/rustc_middle/src/mir/statement.rs Outdated
@jieyouxu

jieyouxu commented Sep 26, 2026

Copy link
Copy Markdown
Member

r? mir-opt

@rustbot rustbot assigned oli-obk and unassigned jieyouxu Sep 26, 2026
@mu001999
mu001999 force-pushed the fix-162220 branch 2 times, most recently from 4718942 to 8530d03 Compare September 27, 2026 03:29
@dianqk dianqk self-assigned this Sep 27, 2026
@dianqk

dianqk commented Sep 27, 2026

Copy link
Copy Markdown
Member

I think we could have a canonicalization format which would benefit other passes. For instance, if there is a place and a constant, the first one should be constant.

Rules:

  • op(place, const) -> op(const, place)
  • op(const, place) -> op(const, place)
  • op(_1.proj, _2.proj) -> op(_1.proj, _2.proj)
  • op(_2.proj, _1.proj) -> op(_1.proj, _2.proj)

When one operand is a constant and the other is a place, we only need to consider op(const, place).

@mu001999

mu001999 commented Sep 27, 2026

Copy link
Copy Markdown
Member Author

@dianqk do you think we should implement it in new pass instead of the GVN pass?

@oli-obk oli-obk removed their assignment Sep 28, 2026
@dianqk

dianqk commented Sep 28, 2026

Copy link
Copy Markdown
Member

@dianqk do you think we should implement it in new pass instead of the GVN pass?

How about adding this to InstSimplify, along with a helper function similar to simplify_cfg for other passes? Some passes may introduce non-canonical instructions, and we can use the helper in GVN to ensure these instructions are canonicalized.

An MCP isn't required in my case, but we should document it in the BinOp docs.

@mu001999

mu001999 commented Sep 28, 2026

Copy link
Copy Markdown
Member Author

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2026
@mu001999
mu001999 marked this pull request as draft September 28, 2026 16:51
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@mu001999 mu001999 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2026
@mu001999

mu001999 commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Update:

  1. Moved canonicalize_binary_operands to a separate canonicalization.rs file.
  2. Changed the canonical order to op(place, const).
  3. For op(place1, place2), only compare the places, ignoring copy/move distinctions. GVN also only cares about the places rather than copy/move, so this should be consistent with GVN.
  4. For arithmetic operations, only perform canonicalization on integer types.

I was just thinking that canonicalize_binary_operands could be more general.

For now, canonicalize_binary_operands still only canonicalizes the order of operands. I'd prefer to expand the scope of canonicalization in follow-up PRs:

  1. Canonicalize other comparison operators, e.g., b > a -> a < b and b >= a -> a <= b.
  2. Canonicalize equivalent arithmetic operations, e.g., a * (2 ^ N) -> a << N.
  3. Move some existing local algebraic simplifications in GVN into canonicalization, e.g., a + 0 -> a, a * 0 -> 0, and a * 1 -> a.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2026
dianqk
dianqk reviewed Oct 17, 2026
View reviewed changes
Comment thread tests/mir-opt/gvn_commutative.rs Outdated
Comment thread tests/mir-opt/gvn_commutative.rs Outdated
@rustbot

rustbot commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@dianqk

dianqk commented Oct 17, 2026

Copy link
Copy Markdown
Member

@bors r+
Thanks!

@rust-bors

rust-bors Bot commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 7ef3170 has been approved by dianqk

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 17, 2026
jhpratt added a commit to jhpratt/rust that referenced this pull request Oct 17, 2026
@jhpratt
Rollup merge of rust-lang#163373 - mu001999-contrib:fix-162220, r=dianqk
1b88e09 
Canonicalize operands for commutative binary ops

Fixes rust-lang#162220
@jhpratt jhpratt mentioned this pull request Oct 17, 2026
Closed
rust-bors Bot pushed a commit that referenced this pull request Oct 17, 2026
@bors
Auto merge of #164460 - jhpratt:rollup-vcfZluk, r=jhpratt
aaf1db3 
Rollup of 10 pull requests

Successful merges:

 - #161698 (Fix rmeta-link metadata not being read on AIX)
 - #163373 (Canonicalize operands for commutative binary ops)
 - #164016 (Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs`)
 - #164424 (diagnostic: skip wrong loops when explaining borrow errors)
 - #138744 (Add methods to TCP and UDP sockets to modify hop limit)
 - #162444 (Make `std::fs::{File, ReadDir, DirEntry}` always `needs_drop` even when unsupported.)
 - #163862 (Collect all limit attributes during TyCtxt creation)
 - #164213 (make the safety documentation consistent in task::LocalWaker)
 - #164356 (Add missing full stop)
 - #164374 (core: hint: fix `unlikely`'s example polarity typo)
rust-bors Bot pushed a commit that referenced this pull request Oct 17, 2026
@bors
Auto merge of #164460 - jhpratt:rollup-vcfZluk, r=jhpratt
bb1aa39 
Rollup of 10 pull requests

Successful merges:

 - #161698 (Fix rmeta-link metadata not being read on AIX)
 - #163373 (Canonicalize operands for commutative binary ops)
 - #164016 (Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs`)
 - #164424 (diagnostic: skip wrong loops when explaining borrow errors)
 - #138744 (Add methods to TCP and UDP sockets to modify hop limit)
 - #162444 (Make `std::fs::{File, ReadDir, DirEntry}` always `needs_drop` even when unsupported.)
 - #163862 (Collect all limit attributes during TyCtxt creation)
 - #164213 (make the safety documentation consistent in task::LocalWaker)
 - #164356 (Add missing full stop)
 - #164374 (core: hint: fix `unlikely`'s example polarity typo)
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Oct 17, 2026
@JonathanBrouwer
Rollup merge of rust-lang#163373 - mu001999-contrib:fix-162220, r=dianqk
427e44f 
Canonicalize operands for commutative binary ops

Fixes rust-lang#162220
This was referenced Oct 17, 2026
Closed
Merged
rust-bors Bot pushed a commit that referenced this pull request Oct 17, 2026
@bors
Auto merge of #164479 - JonathanBrouwer:rollup-0awAwS1, r=JonathanBro…
8006091 
…uwer

Rollup of 11 pull requests

Successful merges:

 - #162809 (Remap local def ids for deterministic metadata encoding)
 - #163373 (Canonicalize operands for commutative binary ops)
 - #164016 (Tighten (absence of) return attribute checks in `some-abis-do-extend-params-to-32-bits.rs`)
 - #164259 (std: make SOLID `sockaddr_storage` large enough for IPv6)
 - #164424 (diagnostic: skip wrong loops when explaining borrow errors)
 - #138744 (Add methods to TCP and UDP sockets to modify hop limit)
 - #162444 (Make `std::fs::{File, ReadDir, DirEntry}` always `needs_drop` even when unsupported.)
 - #163862 (Collect all limit attributes during TyCtxt creation)
 - #164213 (make the safety documentation consistent in task::LocalWaker)
 - #164356 (Add missing full stop)
 - #164374 (core: hint: fix `unlikely`'s example polarity typo)
@rust-bors
rust-bors Bot merged commit 8c955a1 into rust-lang:main Oct 17, 2026
14 checks passed
@rustbot rustbot added this to the 1.101.0 milestone Oct 17, 2026
rust-bors Bot pushed a commit that referenced this pull request Oct 17, 2026
@bors
Unrolled build for #163373 in rollup 164479
73ff8a6 
Rollup merge of #163373 - mu001999-contrib:fix-162220, r=dianqk

Canonicalize operands for commutative binary ops

Fixes #162220
@mu001999
mu001999 deleted the fix-162220 branch October 20, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dianqk dianqk dianqk left review comments
@jieyouxu jieyouxu jieyouxu left review comments

Assignees

@dianqk dianqk

Labels

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.101.0

Development

Successfully merging this pull request may close these issues.

MIR GVN does not eliminate duplicate commutative expressions with swapped operands

6 participants

@mu001999 @rustbot @rust-log-analyzer @jieyouxu @dianqk @oli-obk