[opt] Optimize commutative op working on (explicitly) commuted operands. #3640
+154
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdleary
changed the title
cdleary
added a commit
to xlsynth/xlsynth
that referenced
this pull request
Jan 2, 2026
…itly) commuted operands. [opt] Optimize commutative op operating on explicitly commuted operands.
Contributor
|
This seems good to me... but continues to suggest that we may need better pass organization, since I'm not sure this should qualify as "basic". |
ericastor
approved these changes
Jan 2, 2026
angelomatni1
reviewed
Jan 5, 2026
| // For `sel(p, cases=[on_false, on_true])`, the selector is `p`. | ||
| // For a binary `priority_sel(p, cases=[on_true], default=on_false)`, the | ||
| // selector is `p` (which is required to be a single bit). | ||
| struct BinarySelectArms { |
Contributor
There was a problem hiding this comment.
Maybe rename to BinarySelect since this includes both the arms and the selector?
| XLS_RET_CHECK_EQ(sel->selector()->BitCountOrDie(), 1); | ||
| return BinarySelectArms{.selector = sel->selector(), | ||
| .on_false = sel->get_case(0), | ||
| .on_true = sel->get_case(1)}; |
Contributor
There was a problem hiding this comment.
Is it possible for the select to have a single case and the default value to cover the case for selector == 0b1?
| BValue selector = fb.Param("p", p->GetBitsType(1)); | ||
| BValue a = fb.Param("a", p->GetBitsType(32)); | ||
| BValue b = fb.Param("b", p->GetBitsType(32)); | ||
| BValue sel = fb.Select(selector, {a, b}); |
Contributor
There was a problem hiding this comment.
Unused variable sel; same for the below 2 unit tests
| EXPECT_THAT(f->return_value(), m::Eq(m::Param("a"), m::Param("b"))); | ||
| } | ||
|
|
||
| TEST_F(BasicSimplificationPassTest, AddWithZero) { |
Contributor
There was a problem hiding this comment.
Can we add a unit test on a function that is non-commutative?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
op(sel(p, cases=[a, b]), sel(p, cases=[b, a]))can be optimized toop(a, b)whenopis commutative.X(sel(p,[a,b]), sel(p,[b,a])) -> X(a,b)(applies to any 2-operand commutative, non-side-effecting op).select_simplification_passhas several 2-way-select peepholes (case swapping / collapsing nested selects / detecting binary select patterns), but not this "commutative op of swapped cases collapses" rule.