[FIRRTL] no back-prop for width of mux selectors, support narrower

[dtzSiFive]

Support parsing mux with selector of zero-width, per spec.

Fix use as mux selector "back-propagating" (acts as-if connected to from a 1-bit source).

Canonicalize narrower selectors to full width for lowering and analysis simplicity.

Fixes #5444.

[seldridge]

This would be good to spot check with internal designs see if anything breaks and if there are unexpected Verilog diffs. (The other direction is trying to ratchet this up on the Chisel side by requiring Bool as an input to the mux select line as you suggested. 😉) I'm generally curious how many places this is relied on.

[dtzSiFive]

Ran on few internal designs and so far the verilog is identical 👍 .

[dtzSiFive]

The constraint change here goes from adding sel >= 1 to sel <= 1 which isn't used for solving but is checked for correctness.

[seldridge]

This seems reasonable. One question/concern about if the canonicalization is mandatory for InferWidths.

[seldridge]

If this canonicalization doesn't happen, will this error in InferWidths? I.e., is this illegal FIRRTL?

[dtzSiFive]

This is now legal FIRRTL (ODS change), this helps with things like LowerToHW not knowing how to deal with this sort of selector and generally needing better handling for zero-width things, and to allow participating in/composing with other canonicalizations (e.g., existing canonicalization for constant-false selector, which is what is tested here).

InferWidths runs before canonicalization (so for sure this won't happen until after InferWidths), but it's not illegal if this doesn't run at any point. It just helps clean things up and accommodate brittle passes.

[seldridge]

Assumedly, the asymmetry of the mux4 getting canonicalized to a pad will be immediately cleaned up by a canonicalization that converts pad(_: u0, x) -> 0: u<x>?

[dtzSiFive]

That's the idea but such a canonicalization doesn't exist presently. I've started working on adding this dialect-wide because I'm tired of playing (or seeing others play) whack-a-mole with zero-width anything (WIP/TBD) but yeah that's the idea. And the 1bit is still a pad -- which maybe is something a mux4 could handle and become a mux2 or something (?). Anyway presently Mux4 is missing all of the mux2 folding logic, FWIW.

FWIW, honestly, re:asymmetry -- replacing it with a pad uniformly is what I reached for first -- one thing at a time, but since the pattern doesn't exist just inserted the constant for the 2-mux directly.

I don't think this is a problem (or at least okay to handle separately), the extension used to (/currently, without this PR) implicitly in the connect which is canonicalized to the same pad anyway, consider:

firrtl.circuit "MuxIntrinsics" {
  firrtl.module @MuxIntrinsics(in %sel_0w: !firrtl.uint<0>, in %sel_1w: !firrtl.uint<1>, in %high: !firrtl.uint<1>, in %low: !firrtl.uint<1>, out %out1: !firrtl.uint, out %out2: !firrtl.uint) {
    %c3_ui4 = firrtl.constant 3 : !firrtl.uint<4>
    %c3_ui3 = firrtl.constant 3 : !firrtl.uint<3>
    %c2_ui2 = firrtl.constant 2 : !firrtl.uint<2>
    %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
    %c1_ui2 = firrtl.constant 1 : !firrtl.uint<2>
    %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
    %c1 = firrtl.constant 0: !firrtl.uint
    %sel = firrtl.wire : !firrtl.uint
    firrtl.connect %sel, %sel_0w : !firrtl.uint, !firrtl.uint<0>
    %0 = firrtl.int.mux2cell(%sel, %c0_ui1, %c1) : (!firrtl.uint, !firrtl.uint<1>, !firrtl.uint) -> !firrtl.uint
    firrtl.connect %out1, %0: !firrtl.uint, !firrtl.uint
    %sel2 = firrtl.wire : !firrtl.uint
    firrtl.connect %sel2, %sel_1w : !firrtl.uint, !firrtl.uint<1>
    %1 = firrtl.int.mux4cell(%sel2, %c1_ui1, %c2_ui2, %c3_ui3, %c1) : (!firrtl.uint, !firrtl.uint<1>, !firrtl.uint<2>, !firrtl.uint<3>, !firrtl.uint) -> !firrtl.uint
    firrtl.connect %out2, %1: !firrtl.uint, !firrtl.uint
  }
}

Which if you feed to today's firtool and look at the IR after canonicalize runs, you get:

firrtl.module @MuxIntrinsics(in %sel_0w: !firrtl.uint<0>, in %sel_1w: !firrtl.uint<1>, in %high: !firrtl.uint<1>, in %low: !firrtl.uint<1>, out %out1: !firrtl.uint<1>, out %out2: !firrtl.uint<3>) {
  %c3_ui3 = firrtl.constant 3 : !firrtl.uint<3>
  %c2_ui2 = firrtl.constant 2 : !firrtl.uint<2>
  %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
  %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
  %sel = firrtl.wire : !firrtl.uint<1>
  %0 = firrtl.pad %sel_0w, 1 : (!firrtl.uint<0>) -> !firrtl.uint<1>
  firrtl.strictconnect %sel, %0 : !firrtl.uint<1>
  firrtl.strictconnect %out1, %c0_ui1 : !firrtl.uint<1>
  %sel2 = firrtl.wire : !firrtl.uint<2>
  %1 = firrtl.pad %sel_1w, 2 : (!firrtl.uint<1>) -> !firrtl.uint<2>
  firrtl.strictconnect %sel2, %1 : !firrtl.uint<2>
  %2 = firrtl.int.mux4cell(%sel2, %c1_ui1, %c2_ui2, %c3_ui3, %c0_ui1) : (!firrtl.uint<2>, !firrtl.uint<1>, !firrtl.uint<2>, !firrtl.uint<3>, !firrtl.uint<1>) -> !firrtl.uint<3>
  firrtl.strictconnect %out2, %2 : !firrtl.uint<3>
}

That is to say this is at worst no worse than what we have today.

[seldridge]

Sounds good. Yeah, I was thinking of using pad for the Mux and Mux2 canonicalizations, as well. However, it seems reasonable to just squash those when you see them.

[dtzSiFive]

TYVM for taking a look and the great feedback!

[dtzSiFive]

Spec change this is supporting, FWIW: chipsalliance/firrtl-spec#122 .