Add a few mux-of-const and reg-of-and/or canonicalizers. #8307

rwy7 · 2025-03-07T22:32:29Z

This PR adds four new canonicalization patterns to FIRRTL.

mux(cond, 0, b) -> and(not(cond), b)
mux(cond, 1, b) -> or(cond, b)
mux(cond, a, 0) -> and(cond, a)
mux(cond, a, 1) -> or(not(cond), a)

These canonicalizations are already present for the comb dialect, but we want to run these canonicalizers before lowering layers, which can obscure constant ops behind ports.

The problem with these mux canonicalizers is, they conflict with a register canonicalizer. This register canonicalizer converts a register to a constant, if the register's next-value is a mux of either the register itself, or a constant. For example:

connect(reg, mux(reset, 0, reg)) ==> reg -> 0

These new canonicalizers would transform the connect to:

connect(reg, and(not(reset), reg))

...which prevents the register canonicalizer from running. To get this behaviour back, this PR adds four additional canonicalizations for both registers and reg resets: For registers, the canonicalizers are:

connect(reg, and(reg, x)) ==> reg -> 0
connect(reg, and(x, reg)) ==> reg -> 0

connect(reg, or(reg, x)) ==> reg -> 1
connect(reg, or(x, reg)) ==> reg -> 1

For regresets, we have the same canonicalizers, but with an additional check: the reset value must be a constant zero or one.

reset(reg) = 0 ==> connect(reg, and(reg, x)) ==> reg -> 0
reset(reg) = 0 ==> connect(reg, and(x, reg)) ==> reg -> 0

reset(reg) = 1 ==> connect(reg, or(reg, x)) ==> reg -> 1
reset(reg) = 1 ==> connect(reg, or(x, reg)) ==> reg -> 1

These canonicalizations are already present in comb, but I am adding them to the firrtl dialect so that they can run before lower layers. We generally pool constants at the top of a module. If constants are used within a layerblock, they can be captured as ports during lower layers, which in turn prevents comb canonicalizers from running. Ideally, layer-sink would aggressively clone or sink constants into layerblocks, but we're not there yet. These canonicalizations are important because they can drastically simplify the if/else branches which drive registers. When lowering a seq.firreg to sv, we incorporate any muxes feeding into the register as if-else branches, which conditionally drive the register. These canonicalizations can transform a deeply nested mux chain into a tree of and/or expressions, which reduces the depth of if/else branches

seldridge

The symmetry of these may motivate a canonical mux representation. I.e., this could be written as four canonicalizers for each of the four permutations of where a single binary constant could appear or as two canonicalizers for two of the permutations and then one canonicalizer that puts constants in "canonical" position. "Constants last" is the canonical form chosen below:

mux(cond, a, 0) -> and(cond, a)
mux(cond, a, 1) -> or(not(cond), a)
mux(cond, constant, nonconstant) -> mux(~cond, nonconstant, constant)

Do we already have a canonical form form muxes?

I think the regreset canonicalizers may only be useful for async reset registers as I would expect that the sync reset regreset registers will eventually go through normal mux canonicalization. However, this isn't a great reason to not have both.

These canonicalizers make sense to me. Have you gotten any data from internal designs?

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

seldridge · 2025-03-14T14:58:19Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+
+    return failure();
+  }
+};


Challenge 1: Can this be written in using ODS?

I don't know if that makes sense, since this isn't so much a pattern on the register op, but more, a pattern on the connect driving the register. Maybe I could do it using some new constraints. I took a quick look around and it doesn't look like we do this kind of thing using ODS. I'm interested in suggestions though.

I see, the problem isn't in generating the match, but the replacement. The match would be something like: ConnectOp(RegResetOp(...), and(RegResetOp(...), _) (where the registers are the same). It's that the replacement isn't a replacement of the ConnectOp, but of the RegResetOp. Yeah, I'm not sure exactly how to do this.

seldridge · 2025-03-14T14:58:34Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+  }
+
+  return failure();
+}


Challenge 2: Can this be written in terms of ODS?

This commit adds four new canonicalization patterns to FIRRTL. mux(cond, 0, b) -> and(not(cond), b) mux(cond, 1, b) -> or(cond, b) mux(cond, a, 0) -> and(cond, a) mux(cond, a, 1) -> or(not(cond), a) These canonicalizations are already present for the comb dialect, but we want to run these canonicalizers before lowering layers, which can obscure constant ops behind ports. The problem with these mux canonicalizers is, they conflict with a register canonicalizer. This register canonicalizer converts a register to a constant, if the register's next-value is a mux of either the register itself, or a constant. For example: connect(reg, mux(reset, 0, reg)) ==> reg -> 0 These new canonicalizers would transform the connect to: connect(reg, and(not(reset), reg)) ...which prevents the register canonicalizer from running. To get this behaviour back, this PR adds four additional canonicalizations for both registers and reg resets: For registers, the canonicalizers are: connect(reg, and(reg, x)) ==> reg -> 0 connect(reg, and(x, reg)) ==> reg -> 0 connect(reg, or(reg, x)) ==> reg -> 1 connect(reg, or(x, reg)) ==> reg -> 1 For regresets, we have the same canonicalizers, but with an additional check: the reset value must be a constant zero or one. reset(reg) = 0 ==> connect(reg, and(reg, x)) ==> reg -> 0 reset(reg) = 0 ==> connect(reg, and(x, reg)) ==> reg -> 0 reset(reg) = 1 ==> connect(reg, or(reg, x)) ==> reg -> 1 reset(reg) = 1 ==> connect(reg, or(x, reg)) ==> reg -> 1

rwy7 · 2025-03-20T01:35:37Z

The symmetry of these may motivate a canonical mux representation. I.e., this could be written as four canonicalizers for each of the four permutations of where a single binary constant could appear or as two canonicalizers for two of the permutations and then one canonicalizer that puts constants in "canonical" position. "Constants last" is the canonical form chosen below:
mux(cond, a, 0) -> and(cond, a)
mux(cond, a, 1) -> or(not(cond), a)
mux(cond, constant, nonconstant) -> mux(~cond, nonconstant, constant)
Do we already have a canonical form form muxes?

This is a cool idea! I don't see anything like this, but I do see a conflicting canonicalization in comb:

mux(!p, x, y) -> mux(p, y, x)

(this canonicalization is missing in FIRRTL)

Usually our canonicalizers are reducing the cost of a computation, but negating the condition would increase the cost. Maybe, putting constants last only makes sense when the op is symmetric and can freely rearrange its operands.

rwy7 · 2025-03-20T01:45:06Z

Have you gotten any data from internal designs?

For the most part, these canonicalizers just shuffle around the source-info comments and variable spilling in the verilog. There are a few places where the expressions are different but not meaningfully so, and I've only spotted one place where a mux actually was optimized away.

seldridge · 2025-03-20T01:55:10Z

Usually our canonicalizers are reducing the cost of a computation, but negating the condition would increase the cost.

Yes, this would be bad. Given that, I think the right approach is to keep the four canonicalizers and, in a follow-on, add the mux(!cond, a, b) -> mux(cond, b, a).

seldridge

LGTM

seldridge · 2025-04-02T19:30:42Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+///   reset(reg) = 0 ==> connect(reg, and(reg, x)) ==> reg -> 0
+///   reset(reg) = 0 ==> connect(reg, and(x, reg)) ==> reg -> 0
+///   reset(reg) = 1 ==> connect(reg, or(reg, x))  ==> reg -> 1
+///   reset(reg) = 1 ==> connect(reg, or(x, reg))  ==> reg -> 1


Mega-nit: This syntax is bothering me and took me a while to figure it out. The ==> is an "AND" in the first use and is "is converted to" (or "IMPLIES"?) in the second.

Ah OK I can delete these comments, I don't think they're helpful. FWIW ==> is implication, -> is canonicalization.

seldridge · 2025-04-02T19:33:17Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+      return failure();
+
+    // This canonicalization only applies when the register holds 1 bit.
+    auto type = dyn_cast<UIntType>(op.getResult().getType());


I think SIntType would work, too?

seldridge · 2025-04-02T19:34:44Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+    // This canonicalization only applies when the reset is a constant.
+    auto reset =
+        dyn_cast_or_null<ConstantOp>(op.getResetValue().getDefiningOp());
+    if (!reset)
+      return failure();


Nit: please be exact. This is the reset value and not the reset (or exactly, the reset signal).

seldridge · 2025-04-02T19:37:29Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+  LogicalResult matchAndRewrite(RegResetOp op,
+                                PatternRewriter &rewriter) const override {


Supreme nit:

Suggested change

LogicalResult matchAndRewrite(RegResetOp op,

PatternRewriter &rewriter) const override {

LogicalResult matchAndRewrite(RegResetOp reg,

PatternRewriter &rewriter) const override {

When reading the later rewrite pattern, I found it clearer to use the specific "reg" instead of the generic "op".

seldridge · 2025-04-02T19:38:07Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+/// Justification: The initial value of a register is indeterminant, which means
+/// We are free to choose any initial value when optimizing the circuit. For the
+/// AND patterns, if we assume the initial value is zero, then the register will
+/// always be zero. For the OR patterns, if we assume the initial value is one,
+/// then the register will always be one.


Great for including this on both rewrite patterns! Providing this as a comment makes it clear not only what the pattern is doing, but why and with what justification in the spec we are using. 💯

rwy7 force-pushed the mux-canonicalizers branch 2 times, most recently from 6614bdc to ca9b8c7 Compare March 11, 2025 17:22

rwy7 marked this pull request as ready for review March 14, 2025 01:38

rwy7 requested review from darthscsi and seldridge as code owners March 14, 2025 01:38

seldridge reviewed Mar 14, 2025

View reviewed changes

rwy7 force-pushed the mux-canonicalizers branch from ca9b8c7 to c32d32c Compare March 14, 2025 15:08

rwy7 force-pushed the mux-canonicalizers branch from c32d32c to ac0bead Compare March 14, 2025 16:55

rwy7 added the FIRRTL Involving the `firrtl` dialect label Apr 2, 2025

rwy7 requested a review from seldridge April 2, 2025 18:38

seldridge approved these changes Apr 2, 2025

View reviewed changes

		LogicalResult matchAndRewrite(RegResetOp op,
		PatternRewriter &rewriter) const override {

Add a few mux-of-const and reg-of-and/or canonicalizers. #8307

Are you sure you want to change the base?

Add a few mux-of-const and reg-of-and/or canonicalizers. #8307

Uh oh!

Conversation

rwy7 commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seldridge left a comment

Choose a reason for hiding this comment

Uh oh!

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!

rwy7 commented Mar 20, 2025

Uh oh!

rwy7 commented Mar 20, 2025

Uh oh!

seldridge commented Mar 20, 2025

Uh oh!

seldridge 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!

Uh oh!

rwy7 commented Mar 7, 2025 •

edited

Loading