SyncReadMem ruw (ReadUnderWrite) parameter has no effect

[maxbjurling]

Type of issue: Bug Report

Please provide the steps to reproduce the problem:
Generate verilog for the code below using different values for the last parameter (ruw) of the SyncReadMem call

class MemTest extends Module (){
  val io = IO(new Bundle {
    val wrEn   = Input(Bool())
    val wrAddr = Input(UInt(8.W))
    val wrData = Input(UInt(8.W))
    val rdEn   = Input(Bool())
    val rdAddr = Input(UInt(8.W))
    val rdData = Output(UInt(8.W))
  })
  /* try using SyncReadMem.ReadFirst or SyncReadMem.WriteFirst here */ 
  val m = SyncReadMem(256, UInt(8.W), SyncReadMem.Undefined) 
  when (io.wrEn) {
    m.write(io.wrAddr, io.wrData)
  }
  io.rdData := m.read(io.rdAddr, io.rdEn)
}

What is the current behavior?
The generated verilog code always behaves as WriteFirst. If a memory entry is read and written in the same clock cycle, the read data gets the newly written value in the next cycle.

The important parts of the generated verilog code is:

always @(posedge R0_clk) begin
  _R0_en_d0 <= R0_en;
  _R0_addr_d0 <= R0_addr;
end
always @(posedge W0_clk) begin
  if (W0_en & 1'h1)
    Memory[W0_addr] <= W0_data;
end
assign R0_data = _R0_en_d0 ? Memory[_R0_addr_d0] : 8'bx;

What is the expected behavior?
The generated verilog should behave as specified by the ruw parameter, or as described in the Chisel Memories documentation when ruw is not specified.

Please tell us about your environment:

• Chisel version: 6.5
• SBT version 1.9.8

Other Information

What is the use case for changing the behavior?
It should be possible to simulate SRAMs with different read/write conflict behaviour without replacing the memories with vendor models.

[seldridge]

This was never implemented in CIRCT and, hence, this always incorrectly compiles to write first. There's an old tracking issue here: llvm/circt#787

CIRCT passes should handle this if we are going to support it or the option should be dropped from Chisel. It shouldn't be that bad to just support it in CIRCT.

[sequencer]

I think we should remove or at least deprecate this and back port as bug fix to avoid misunderstanding.