Mixed-masked/unmasked-port ReadSyncMem is generated incorrectly when "--repl-seq-mem"

[zhutmost]

Type of issue: Bug Report

Please provide the steps to reproduce the problem:

I have attached two write ports to a ReadSyncMem. One write port has a mask, and the other one does not have a mask.

package mylib

import circt.stage.ChiselStage

import chisel3._
import chisel3.util._

class Example extends Module {
  val wen  = IO(Input(Bool()))
  val wen2 = IO(Input(Bool()))
  val ren  = IO(Input(Bool()))
  val addr = IO(Input(UInt(32.W)))
  val mask = IO(Input(Vec(2, Bool())))
  val din  = IO(Input(Vec(2, UInt(8.W))))
  val dou  = IO(Output(Vec(2, UInt(8.W))))

  withClockAndReset(clock, reset) {
    val uExampleArray = SyncReadMem(32, din.cloneType, SyncReadMem.ReadFirst)
    uExampleArray.suggestName("ArrayA")
    when(wen) {
      uExampleArray.write(addr, din, mask)
    }
    when(wen2) {
      uExampleArray.write(addr, din)
    }
    dou := uExampleArray.read(addr, ren)
  }
}

object Test extends App {
  val targetDir = "generate"

  val chiselArgs: Array[String] = Array(f"--target-dir=$targetDir", "--split-verilog")
  val firtoolArgs: Array[String] = Array(
    // "--disable-all-randomization",
    f"-o=$targetDir",
    "-repl-seq-mem",
    "-repl-seq-mem-circuit=MyChipTop",
    "-repl-seq-mem-file=MyChipTop.sv.conf"
  )
  ChiselStage
    .emitSystemVerilogFile(new Example, chiselArgs, firtoolArgs)
  println(">>> RTL emitted in \"generate\" directory.")
}

What is the current behavior?

The generated Verilog is shown as follows. The input signal mask_X are totally dropped and unused. The generate circuits and the original chisel code have different functions.

// Generated by CIRCT firtool-1.40.0
module Example( // <stdin>:3:10
  input         clock,  // <stdin>:4:11
                reset,  // <stdin>:5:11
                wen,    // mylib/src/Test.scala:9:16
                wen2,   // mylib/src/Test.scala:10:16
                ren,    // mylib/src/Test.scala:11:16
  input  [31:0] addr,   // mylib/src/Test.scala:12:16
  input         mask_0, // mylib/src/Test.scala:13:16
                mask_1, // mylib/src/Test.scala:13:16
  input  [7:0]  din_0,  // mylib/src/Test.scala:14:16
                din_1,  // mylib/src/Test.scala:14:16
  output [7:0]  dou_0,  // mylib/src/Test.scala:15:16
                dou_1   // mylib/src/Test.scala:15:16
);

  wire [15:0] _ArrayA_R0_data;  // mylib/src/Test.scala:18:36
  wire [15:0] _GEN = {din_1, din_0};    // mylib/src/Test.scala:18:36
  ArrayA ArrayA (   // mylib/src/Test.scala:18:36
    .R0_addr (addr[4:0]),   // mylib/src/Test.scala:26:30
    .R0_en   (ren),
    .R0_clk  (clock),
    .W0_addr (addr[4:0]),   // mylib/src/Test.scala:21:26
    .W0_en   (wen),
    .W0_clk  (clock),
    .W0_data (_GEN),    // mylib/src/Test.scala:18:36
    .W1_addr (addr[4:0]),   // mylib/src/Test.scala:24:26
    .W1_en   (wen2),
    .W1_clk  (clock),
    .W1_data (_GEN),    // mylib/src/Test.scala:18:36
    .R0_data (_ArrayA_R0_data)
  );
  assign dou_0 = _ArrayA_R0_data[7:0];  // <stdin>:3:10, mylib/src/Test.scala:18:36
  assign dou_1 = _ArrayA_R0_data[15:8]; // <stdin>:3:10, mylib/src/Test.scala:18:36
endmodule

The generated --repl-seq-mem conf file "MyChipTop.sv.conf" as well as metadata/seq_mems.json are also wrong.

What is the expected behavior?

1. Exit when compliation/elaboration, and throw an error message like "Chisel cannot mix masked and unmasked ports."
2. Generate a memory with two masked write ports. and one is connected to mask, and the other is set to 0 ( means always enable ).

Please tell us about your environment: Firtool 1.40 Chisel 5.0.0-RC2 on macOS, OpenJDK17.

Other Information

What is the use case for changing the behavior?

[seldridge]

I think this is stemming from this being recently allowed in CIRCT and this specific code path never being possible before.

This used to not be a candidate for memory replacement, but is after https://github.com/llvm/circt/pull/4850.

CC: @sequencer, @SpriteOvO. Is this something that affects you and can take a look?

[SpriteOvO]

I have discussed this with @sequencer and there are indeed some bugs. I'm investigating it :/

For the expected behaviors in the OP, we think it shouldn't throw an error, the second behavior makes more sense.

[sequencer]

To be more specific, The main problem is the second read doesn't know which port should bind to? I think in the future we should resolve this with intrinsic

[zhutmost]

To be more specific, The main problem is the second read doesn't know which port should bind to? I think in the future we should resolve this with intrinsic

Hi @sequencer . The read port looks good. the masked write port is generated wrongly.

[sequencer]

Yes, but which port should this read bind to?

[SpriteOvO]

@seldridge Hi, I revert my PR llvm/circt/pull/4850 locally, and the problem from OP is still reproducible (2 unused masks), seems like it was not introduced from my PR.

Should we continue to fix it or wait for the new memory model? If we go to the former, and if I understand correctly, the fix will be applied in LoweringMemory pass?

[poemonsense]

I'm facing the same issue here in Chisel 6.4.0. Since Chisel does not restrict the usage of SyncReadMem to single-port only, I think Chisel should at least ensure the correct semantics of generated Verilog. Even if the generated code is mostly not possible for ASIC, users may use SyncReadMem to construct simulation models. Is this issue expected to be fixed in the near future?

[poemonsense]

A temporary workaround seems adding a mask Wire with dontTouch and connecting it to the unmasked write port.

[poemonsense]

@sequencer @SpriteOvO Any progress on this issue? Or, is there any plan on getting this fixed? Probably https://github.com/chipsalliance/chisel/issues/3542 is a similar issue. Thank you.

[sequencer]

I personally think --repl-seq-mem should not be used in the designs, and need to be marked as deprecated.