[opt] Add masked-xor strength reduction for binary selects by mag-mga · Pull Request #4881 · google/xls

mag-mga · 2026-08-31T13:58:18Z

A binary select of the form sel(p, [x, xor(x, c)]) is rewritten into xor(x, sign_ext(p) & c) when c is fully known, with the selector complemented when the xor arm is the on-false case. The rewrite also applies when an earlier pass (e.g. concat_simp) already split the xor limb into per-slice pieces, as long as the two arms provably differ by a fully known constant (TryFoldXorDelta).

Runs at optimization levels where split-based rewrites are enabled (SplitsEnabled); it is the strength-reduced form of the per-step select in an unrolled CRC/shift loop.

A binary select of the form sel(p, [x, xor(x, c)]) is rewritten into xor(x, sign_ext(p) & c) when c is fully known, with the selector complemented when the xor arm is the on-false case. The rewrite also applies when an earlier pass (e.g. concat_simp) already split the xor limb into per-slice pieces, as long as the two arms provably differ by a fully known constant (TryFoldXorDelta). Runs at optimization levels where split-based rewrites are enabled (SplitsEnabled); it is the strength-reduced form of the per-step select in an unrolled CRC/shift loop.

mag-mga · 2026-08-31T14:05:30Z

Hi, @ericastor, please, take a look at this PR. My optimization seems to be close to your MaybeConvertSelectToMask inside select_simp.

mag-mga · 2026-09-07T05:51:34Z

@ericastor , ping

allight · 2026-09-07T18:14:52Z

@mag-mga please address comments before asking for further review.

Edit: sorry bad github ui. Comments sent

allight

Oh whoops sorry hadn't actually sent these

allight · 2026-08-31T16:08:16Z

                     m::SignExt(m::Eq(m::Param("p"), m::Literal(0b00)))));
 }

+TEST_P(SelectSimplificationPassTest, SelectWithConstantXorCaseCrcShift) {


Please use FunctionBuilder to create tests.

Rewrote to FunctionBuilder

allight · 2026-08-31T16:20:59Z

+// Flattens `node` into its non-concat pieces, most-significant first: the same
+// order in which `concat` lists its operands and `bits_ops::Concat`
+// reassembles them.
+void FlattenConcats(Node* node, std::vector<Node*>& pieces) {


Because both concat_simp and select_simp are in the fixedpoint_simp compound pass this seems like it could cause the optimization to never finish if it triggers (because concat_simp will continuously undo the change).

IIUC, this doesn't actually flatten the concats - it reads through them so the operation can see what it's working on. It still only applies if this optimization actually takes an action, which I don't think is something concat_simp can undo.

Make it as independent select_to_bitwise_ops pass in the second commit

allight · 2026-08-31T17:35:11Z

 //     potentially more efficient components, and exposing further optimization
 //     opportunities.
 //
+// 16. **Converting Binary `Select` to Masked XOR**:


This seems very unlike the rest of the transforms in this pass in that it really depends on the values in the select cases.

This transform should either be in strength_red or more likely in a totally separate pass itself IMO.

(There's also some questions we've been having internally about changing the way that we organize optimization phases which would probably want this to be late in compilation, but that's only peripherally related to this.)

It really is closely related to transform (14) above. I would agree that maybe both of these should be split out into another pass, though.

ericastor · 2026-09-08T03:03:20Z

Apologies - I was at a conference all last week, and then today was a US holiday. I'll pick this up for a proper review in the morning.

ericastor · 2026-09-08T11:46:02Z

The code seems solid, and this is an interesting proposal - though I think I agree with @allight that it's starting to look like it'd be worth splitting out the "select -> bitwise operations" transforms into their own pass. Are you able to share a concrete example of something where this makes a difference to optimized IR quality or to PPA?

mag-mga · 2026-09-10T08:56:16Z

The code seems solid, and this is an interesting proposal - though I think I agree with @allight that it's starting to look like it'd be worth splitting out the "select -> bitwise operations" transforms into their own pass. Are you able to share a concrete example of something where this makes a difference to optimized IR quality or to PPA?

We had a hot CRC function inside many loops (something like that below)

top fn __crc16_attach_single_proc__crc16_step(crc: bits[16] id=1, bit: bits[1] id=2) -> bits[16] {
  bit_slice.14: bits[15] = bit_slice(crc, start=0, width=15, id=14, pos=[(0,9,16)])
  literal.13: bits[1] = literal(value=0, id=13, pos=[(0,9,16)])
  bit_slice.4: bits[1] = bit_slice(crc, start=15, width=1, id=4, pos=[(0,8,20)])
  shifted: bits[16] = concat(bit_slice.14, literal.13, id=15, pos=[(0,9,16)])
  CRC16_POLY: bits[16] = literal(value=4129, id=3, pos=[(0,5,19)])
  feedback: bits[1] = xor(bit_slice.4, bit, id=5, pos=[(0,8,20)])
  xor.8: bits[16] = xor(shifted, CRC16_POLY, id=8, pos=[(0,12,4)])
  ret sel.9: bits[16] = sel(feedback, cases=[shifted, xor.8], id=9, pos=[(0,11,2)])
}

Previously, it generates

module __crc16_attach_single_proc__crc16_step(
  input wire [15:0] crc,
  input wire bit,
  output wire [15:0] out
);
  wire feedback;
  wire [15:0] shifted;
  assign feedback = crc[15] ^ bit;
  assign shifted = {crc[14:0], 1'h0};
  assign out = feedback ? {crc[14:0] ^ 15'h0810, 1'h1} : shifted;
endmodule

Now it leads to

module __crc16_attach_single_proc__crc16_step(
  input wire [15:0] crc,
  input wire bit,
  output wire [15:0] out
);
  wire feedback;
  wire [15:0] shifted;
  assign feedback = crc[15] ^ bit;
  assign shifted = {crc[14:0], 1'h0};
  assign out = shifted ^ {16{feedback}} & 16'h1021;
endmodule

For our target it is especially better for FPGA LUT and sometimes also better in synthesis (although some synthesis tools can finally synthesize both functions in the same way).

I'm not confident I have enough permission to provide PPA :(

ericastor · 2026-09-10T12:59:11Z

The code seems solid, and this is an interesting proposal - though I think I agree with @allight that it's starting to look like it'd be worth splitting out the "select -> bitwise operations" transforms into their own pass. Are you able to share a concrete example of something where this makes a difference to optimized IR quality or to PPA?

We had a hot CRC function inside many loops (something like that below)
top fn __crc16_attach_single_proc__crc16_step(crc: bits[16] id=1, bit: bits[1] id=2) -> bits[16] {
  bit_slice.14: bits[15] = bit_slice(crc, start=0, width=15, id=14, pos=[(0,9,16)])
  literal.13: bits[1] = literal(value=0, id=13, pos=[(0,9,16)])
  bit_slice.4: bits[1] = bit_slice(crc, start=15, width=1, id=4, pos=[(0,8,20)])
  shifted: bits[16] = concat(bit_slice.14, literal.13, id=15, pos=[(0,9,16)])
  CRC16_POLY: bits[16] = literal(value=4129, id=3, pos=[(0,5,19)])
  feedback: bits[1] = xor(bit_slice.4, bit, id=5, pos=[(0,8,20)])
  xor.8: bits[16] = xor(shifted, CRC16_POLY, id=8, pos=[(0,12,4)])
  ret sel.9: bits[16] = sel(feedback, cases=[shifted, xor.8], id=9, pos=[(0,11,2)])
}
...

For our target it is especially better for FPGA LUT and sometimes also better in synthesis (although some synthesis tools can finally synthesize both functions in the same way).

I'm not confident I have enough permission to provide PPA :(

Oh, no worries there! I meant the motivating example, rather than PPA specifics, so I appreciate your sharing it. I'll review this again, and thanks for reworking it as requested.

ericastor

This is great, thank you!

ericastor · 2026-09-10T13:00:13Z

      # This is mostly to clean up extraneous bit-slice/concats etc.
      "fixedpoint_simp",
+      "dce",
+      # Convert binary selects whose arms differ by a fully-known xor term into


As I mentioned, I'm not actually sure this is true - so we'll check it, and decide where it should live overall. Thanks!

ericastor · 2026-09-10T13:06:18Z

+// Returns the (on_false, on_true) arm pair of a single-bit binary mux: either
+// two explicit cases, or one explicit case plus a default value. Returns
+// std::nullopt for any other select shape.
+std::optional<std::pair<Node*, Node*>> GetBinaryMuxArms(Select* sel) {


As a followup, we might want to switch this to use our GenericSelect framework! But for now, this is sensible.

ericastor · 2026-09-10T13:10:54Z

Oh darn, our CI checks are aborting on the lack of a newline at the end of your new files. Are you able to fix that?

I might just do it on import, though.

mag-mga · 2026-09-10T14:33:55Z

Oh darn, our CI checks are aborting on the lack of a newline at the end of your new files. Are you able to fix that?

I might just do it on import, though.

@ericastor ,
I've added newlines as a separate commit

PiperOrigin-RevId: 980085178

mag-mga · 2026-09-15T07:38:52Z

I think we should close this MR because it is already merged to main branch

erinzmoore requested a review from ericastor August 31, 2026 16:28

allight reviewed Sep 7, 2026

View reviewed changes

[opt] make select-to-bitwise-ops as an independent pass

02f1493

mag-mga force-pushed the mag-mga/select-to-xor branch from 7d5b733 to 02f1493 Compare September 10, 2026 08:40

mag-mga requested a review from allight September 10, 2026 08:56

ericastor approved these changes Sep 10, 2026

View reviewed changes

ericastor added the Reviewing Internally label Sep 10, 2026

[lint]: fix formatting

ddb049c

copybara-service Bot pushed a commit that referenced this pull request Sep 11, 2026

Merge pull request #4881 from mag-mga:mag-mga/select-to-xor

49c163e

PiperOrigin-RevId: 980085178

mag-mga closed this Sep 15, 2026

Conversation

mag-mga commented Aug 31, 2026

Uh oh!

mag-mga commented Aug 31, 2026

Uh oh!

mag-mga commented Sep 7, 2026

Uh oh!

allight commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allight left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericastor commented Sep 8, 2026

Uh oh!

ericastor commented Sep 8, 2026

Uh oh!

mag-mga commented Sep 10, 2026

Uh oh!

ericastor commented Sep 10, 2026

Uh oh!

ericastor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericastor commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mag-mga commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mag-mga commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

allight commented Sep 7, 2026 •

edited

Loading

ericastor commented Sep 10, 2026 •

edited

Loading

mag-mga commented Sep 10, 2026 •

edited

Loading