Recursively generate one-hot multiplexers for aggregates

[jerryz123]

The primary motivation for this is to enable optimization passes over elements of an Aggregate that would otherwise be unable to propagate across the UInt conversion.

Type of change: feature request

Impact: no functional change

Development Phase: implementation

Release Notes

[CLAassistant]

All committers have signed the CLA.

[jackkoenig]

Can you show an example of what this changes and write a test to ensure we never regress on it?

[jerryz123]

class Foo extends RawModule {
  val io = IO(new Bundle {
    val sel = Input(Vec(2, Bool()))
    val in = Input(Vec(2, Vec(2, UInt(3.W))))
    val out = Output(UInt(3.W))
  })

  val out = Mux1H(io.sel, io.in)
  io.out := out(0)
}

Chisel would originally generate the following.

circuit Foo : 
  module Foo : 
    output io : {flip sel : UInt<1>[2], flip in : UInt<3>[2][2], out : UInt<3>}
    
    node _T = cat(io.in[0][1], io.in[0][0]) @[Mux.scala 27:72]
    node _T_1 = mux(io.sel[0], _T, UInt<1>("h00")) @[Mux.scala 27:72]
    node _T_2 = cat(io.in[1][1], io.in[1][0]) @[Mux.scala 27:72]
    node _T_3 = mux(io.sel[1], _T_2, UInt<1>("h00")) @[Mux.scala 27:72]
    node _T_4 = or(_T_1, _T_3) @[Mux.scala 27:72]
    wire out : UInt<3>[2] @[Mux.scala 27:72]
    wire _T_5 : UInt<6>
    _T_5 <= _T_4
    node _T_6 = bits(_T_5, 2, 0) @[Mux.scala 27:72]
    out[0] <= _T_6 @[Mux.scala 27:72]
    node _T_7 = bits(_T_5, 5, 3) @[Mux.scala 27:72]
    out[1] <= _T_7 @[Mux.scala 27:72]
    io.out <= out[0] @[Demo.scala 16:10]

module Foo(
  input        io_sel_0,
  input        io_sel_1,
  input  [2:0] io_in_0_0,
  input  [2:0] io_in_0_1,
  input  [2:0] io_in_1_0,
  input  [2:0] io_in_1_1,
  output [2:0] io_out
);
  wire [5:0] _T = {io_in_0_1,io_in_0_0}; // @[Mux.scala 27:72]
  wire [5:0] _T_1 = io_sel_0 ? _T : 6'h0; // @[Mux.scala 27:72]
  wire [5:0] _T_2 = {io_in_1_1,io_in_1_0}; // @[Mux.scala 27:72]
  wire [5:0] _T_3 = io_sel_1 ? _T_2 : 6'h0; // @[Mux.scala 27:72]
  wire [5:0] _T_4 = _T_1 | _T_3; // @[Mux.scala 27:72]
  assign io_out = _T_4[2:0]; // @[Demo.scala 16:10]
endmodule

This change causes Chisel to emit the following instead.

circuit Foo : 
  module Foo : 
    output io : {flip sel : UInt<1>[2], flip in : UInt<3>[2][2], out : UInt<3>}
    
    wire out : UInt<3>[2] @[Mux.scala 27:72]
    node _T = mux(io.sel[0], io.in[0][0], UInt<1>("h00")) @[Mux.scala 27:72]
    node _T_1 = mux(io.sel[1], io.in[1][0], UInt<1>("h00")) @[Mux.scala 27:72]
    node _T_2 = or(_T, _T_1) @[Mux.scala 27:72]
    wire _T_3 : UInt<3> @[Mux.scala 27:72]
    _T_3 <= _T_2 @[Mux.scala 27:72]
    out[0] <= _T_3 @[Mux.scala 27:72]
    node _T_4 = mux(io.sel[0], io.in[0][1], UInt<1>("h00")) @[Mux.scala 27:72]
    node _T_5 = mux(io.sel[1], io.in[1][1], UInt<1>("h00")) @[Mux.scala 27:72]
    node _T_6 = or(_T_4, _T_5) @[Mux.scala 27:72]
    wire _T_7 : UInt<3> @[Mux.scala 27:72]
    _T_7 <= _T_6 @[Mux.scala 27:72]
    out[1] <= _T_7 @[Mux.scala 27:72]
    io.out <= out[0] @[Demo.scala 16:10]
    
module Foo(
  input        io_sel_0,
  input        io_sel_1,
  input  [2:0] io_in_0_0,
  input  [2:0] io_in_0_1,
  input  [2:0] io_in_1_0,
  input  [2:0] io_in_1_1,
  output [2:0] io_out
);
  wire [2:0] _T = io_sel_0 ? io_in_0_0 : 3'h0; // @[Mux.scala 27:72]
  wire [2:0] _T_1 = io_sel_1 ? io_in_1_0 : 3'h0; // @[Mux.scala 27:72]
  assign io_out = _T | _T_1; // @[Demo.scala 16:10]
endmodule

write a test

I could only find tests which test functionality of the generated circuit, rather than properties of the generated FIRRTL. As this change does not affect functionality, how should it be tested?

[jackkoenig]

We have some patterns for checking the generated Verilog, but I'm not sure if it's worth it here. I poked around to see the existing tests; I think it would be nice to make the existing check actually check all of the indices rather than being a mere sanity check: https://github.com/freechipsproject/chisel3/blob/473a13877c60ba9fb13de47542a8397412c2b967/src/test/scala/chiselTests/OneHotMuxSpec.scala#L179

[jerryz123]

I modified the ParameterizedOneHotTesters to test all cases

[jackkoenig]

Looks like my suggestion was wrong, I'll fix it and merge this

[jackkoenig]

Thanks @jerryz123!

[jerryz123]

Thanks for pushing this through, @jackkoenig .