Extmodule port name check fails with length-parameterized Vec ports

[johnsbrew]

Checklist

• Did you specify the current behavior?
• Did you specify the expected behavior?
• Did you provide a code example showing the problem?
• Did you describe your environment?
• Did you specify relevant external information?

What is the current behavior?

https://scastie.scala-lang.org/chXB5p5iTd29RZQwJPZV8A

Given a parameterized blackbox, whose parameter LEN is used as port out Vec length:

class ExampleBB(
    val WIDTH: Int, 
    val LEN: Int
  ) extends BlackBox(Map(
        "WIDTH" -> WIDTH, 
        "LEN" -> LEN
  )) {
  val io = IO(new Bundle {
    val in = Input(UInt(WIDTH.W))
    val out = Output(Vec(LEN, UInt(WIDTH.W)))
  })
}

Instantiating this blackbox twice in the same design will lead to fatal FIRRTL error.

  val lenA = 2
  val lenB = 4

  val inst_a = Module(new ExampleBB(width, lenA))

  val inst_b = Module(new ExampleBB(width, lenB))

FIRRTL raises error on

firrtl.passes.PassExceptions: 
firrtl.passes.CheckHighFormLike$DefnameDifferentPortsException: : ports of extmodule ExampleBB with defname ExampleBB are different for an extmodule with the same defname
firrtl.passes.CheckHighFormLike$DefnameDifferentPortsException: : ports of extmodule ExampleBB_1 with defname ExampleBB are different for an extmodule with the same defname
firrtl.passes.PassException: 2 errors detected!

What is the expected behavior?

module Example(
  input        clock,
  input        reset,
  input  [4:0] in,
  output [4:0] out_a_0,
  output [4:0] out_a_1,
  output [4:0] out_b_0,
  output [4:0] out_b_1,
  output [4:0] out_b_2,
  output [4:0] out_b_3
);
  wire [4:0] inst_a_in; // @[main.scala 30:22]
  wire [4:0] inst_a_out_0; // @[main.scala 30:22]
  wire [4:0] inst_a_out_1; // @[main.scala 30:22]
  wire [4:0] inst_b_in; // @[main.scala 34:22]
  wire [4:0] inst_b_out_0; // @[main.scala 34:22]
  wire [4:0] inst_b_out_1; // @[main.scala 34:22]
  wire [4:0] inst_b_out_2; // @[main.scala 34:22]
  wire [4:0] inst_b_out_3; // @[main.scala 34:22]
  ExampleBB #(.WIDTH(5), .LEN(2)) inst_a ( // @[main.scala 30:22]
    .in(inst_a_in),
    .out_0(inst_a_out_0),
    .out_1(inst_a_out_1)
  );
  ExampleBB #(.WIDTH(5), .LEN(4)) inst_b ( // @[main.scala 34:22]
    .in(inst_b_in),
    .out_0(inst_b_out_0),
    .out_1(inst_b_out_1),
    .out_2(inst_b_out_2),
    .out_3(inst_b_out_3)
  );
  assign out_a_0 = inst_a_out_0; // @[main.scala 32:9]
  assign out_a_1 = inst_a_out_1; // @[main.scala 32:9]
  assign out_b_0 = inst_b_out_0; // @[main.scala 36:9]
  assign out_b_1 = inst_b_out_1; // @[main.scala 36:9]
  assign out_b_2 = inst_b_out_2; // @[main.scala 36:9]
  assign out_b_3 = inst_b_out_3; // @[main.scala 36:9]
  assign inst_a_in = in; // @[main.scala 31:16]
  assign inst_b_in = in; // @[main.scala 35:16]
endmodule

obtained with a simple comment at

firrtl/src/main/scala/firrtl/passes/CheckHighForm.scala

Line 145 in 4347f57

errors.append(new DefnameDifferentPortsException(info, name, defname))

Steps to Reproduce

https://scastie.scala-lang.org/chXB5p5iTd29RZQwJPZV8A

Your environment

• Chisel Version: 3.5.0-RC1 and earlier

How to fix discussion

• Preferred solution: purely remove this check

  1. I can't figure an example in which it would prove useful as various blackbox instances might not necessarily provide the same port map while still resulting in fully-functional verilog
  2. The idea of FIRRTL checks guaranteeing any safety about user-provided blackboxes and their instantiation is quite pretentious anyway
  3. The idea of raising early errors without any grounds (i.e. based on an actual inability to compile the circuit) sounds absurd to me

• Alternative solution: add explicit support for flattened vecs

  • exclude names matching pattern ".+_[0-9]+" from the check

[seldridge]

Thanks for the bug report, @johnsbrew. I have some questions...

I think the output Verilog is actually malformed though. The only way that would be valid is if ExampleBB has undriven ports which is one of the things that this check is trying to avoid. E.g., if you try and run this through Verilator, you'll get pin missing errors. (I defined ExampleBB as having all 5 ports.):

Full definition of `johnsbrew.v`

module ExampleBB
#(
  parameter WIDTH = 5,
  parameter LEN = 4
)
(
  input [4:0] in,
  output [4:0] out_0,
  output [4:0] out_1,
  output [4:0] out_2,
  output [4:0] out_3
);
endmodule


module Example(
  input        clock,
  input        reset,
  input  [4:0] in,
  output [4:0] out_a_0,
  output [4:0] out_a_1,
  output [4:0] out_b_0,
  output [4:0] out_b_1,
  output [4:0] out_b_2,
  output [4:0] out_b_3
);
  wire [4:0] inst_a_in; // @[main.scala 30:22]
  wire [4:0] inst_a_out_0; // @[main.scala 30:22]
  wire [4:0] inst_a_out_1; // @[main.scala 30:22]
  wire [4:0] inst_b_in; // @[main.scala 34:22]
  wire [4:0] inst_b_out_0; // @[main.scala 34:22]
  wire [4:0] inst_b_out_1; // @[main.scala 34:22]
  wire [4:0] inst_b_out_2; // @[main.scala 34:22]
  wire [4:0] inst_b_out_3; // @[main.scala 34:22]
  ExampleBB #(.WIDTH(5), .LEN(2)) inst_a ( // @[main.scala 30:22]
    .in(inst_a_in),
    .out_0(inst_a_out_0),
    .out_1(inst_a_out_1)
  );
  ExampleBB #(.WIDTH(5), .LEN(4)) inst_b ( // @[main.scala 34:22]
    .in(inst_b_in),
    .out_0(inst_b_out_0),
    .out_1(inst_b_out_1),
    .out_2(inst_b_out_2),
    .out_3(inst_b_out_3)
  );
  assign out_a_0 = inst_a_out_0; // @[main.scala 32:9]
  assign out_a_1 = inst_a_out_1; // @[main.scala 32:9]
  assign out_b_0 = inst_b_out_0; // @[main.scala 36:9]
  assign out_b_1 = inst_b_out_1; // @[main.scala 36:9]
  assign out_b_2 = inst_b_out_2; // @[main.scala 36:9]
  assign out_b_3 = inst_b_out_3; // @[main.scala 36:9]
  assign inst_a_in = in; // @[main.scala 31:16]
  assign inst_b_in = in; // @[main.scala 35:16]
endmodule

# verilator -lint-only johnsbrew.v
%Warning-PINMISSING: johnsbrew.v:35:35: Cell has missing pin: 'out_2'
   35 |   ExampleBB #(.WIDTH(5), .LEN(2)) inst_a (  
      |                                   ^~~~~~
                     ... For warning description see https://verilator.org/warn/PINMISSING?v=4.210
                     ... Use "/* verilator lint_off PINMISSING */" and lint_on around source to disable this message.
%Warning-PINMISSING: johnsbrew.v:35:35: Cell has missing pin: 'out_3'
   35 |   ExampleBB #(.WIDTH(5), .LEN(2)) inst_a (  
      |                                   ^~~~~~
%Error: Exiting due to 2 warning(s)

The check is extremely conservative. It's trying to assert that if a user defines a blackbox, that all the blackboxes that the user said are the same are actually the same. This check hinges on one lemma: Verilog does not have optional ports, but may have parametric port widths. One of the following checks is then applied:

1. If the module is not parameteric, then all extmodules with the same defname (meaning these map to the exact same Verilog module) have to have the exact same ports and port widths.
2. If the module is parametric, then all extmodules with the same defname must have the same port names, but widths are allowed to be different because FIRRTL doesn't know if the parameters are used to set the widths of ports.

This is erroring out, because the Chisel code said, "Hey, I have two instances of ExampleBB and one has 3 ports and one has 5 ports. This is impossible due to Verilog restrictions."

It seems like removing this check is allowing circumvention of the guarantee that everything in the circuit is connected to, including extmodules. This seems unsafe.

If these are actually different Verilog modules, then you can set the FIRRTL defname by overriding desiredName like in this modified snippet.

[johnsbrew]

Thanks for your quick answer @seldridge, I drafted this report a few weeks ago, convinced that this issue could be solved very simply, however it seems I overlooked it.

Thanks to your answer I've come back to my actual problem which is the conversion of val out = Output(Vec(LEN, UInt(WIDTH.W))) into multiple out_ signals indexed from 0 until LEN, while the verilog blackbox has a single out signal.

Do we have any chisel-based solution for such case?
While I can wrap the actual blackbox within a module in charge of casting to a large UInt (see scastie example below) to generate proper verilog without changes, it does not really comply with the actual blackbox definition but rather plays with verilog permissiveness to work.
https://scastie.scala-lang.org/p2GUhU3GRiuBd8koWK9h3g

Any thoughts about this underlying issue?

Regarding the relevance of the check itself, I do understand your point of view, which provides some kind of safety in most cases, but I am really not a big fan of failing while being able to generate meaningful verilog.

• Verilog modules might have unused ports (typically unconnected extra, with width parameterized to 0)
• Verilog modules ports might actually exist (or not) due to preprocessor parameters (admittedly irrelevant point as the instances here shall refer the very same pre-processed blackbox)