Dual-port RAM is not synthesized correctly while Single-port does

[carlosedp]

If I add a second set of ports to a BRAM module, the synthesized verilog does not infer as BRAM (in Yosys for example).

Single port:

class SinglePortRAM(sizeKB: Int = 1, width: Int = 32, memoryFile: String = "") extends Module {
  val addrWidth = chiselTypeOf((sizeKB * 1024).U)
  val io = IO(new Bundle {
    val addr1 = Input(addrWidth)
    val dataIn1 = Input(UInt(width.W))
    val en1 = Input(Bool())
    val we1 = Input(Bool())
    val dataOut1 = Output(UInt(width.W))
  })

  val mem = SyncReadMem(sizeKB * 1024, UInt(width.W))
  if (memoryFile.trim().nonEmpty) {
      loadMemoryFromFile(mem, memoryFile)
  }

  when (io.we1) {
    mem.write(io.addr1, io.dataIn1)
  }
  io.dataOut1 := mem.read(io.addr1, io.en1)
}

Generates:

=== SinglePortRAM ===

   Number of wires:                 25
   Number of wire bits:            203
   Number of public wires:          25
   Number of public wire bits:     203
   Number of memories:               0
   Number of memory bits:            0
   Number of processes:              0
   Number of cells:                 22
     DP16KD                          2
     LUT4                           10
     TRELLIS_FF                     10

While Dual port:

class DualPortRAM(sizeKB: Int = 1, width: Int = 32, memoryFile: String = "") extends Module {
  val addrWidth = chiselTypeOf((sizeKB * 1024).U)
  val io = IO(new Bundle {
    // Port 1
    val addr1 = Input(addrWidth)
    val dataIn1 = Input(UInt(width.W))
    val en1 = Input(Bool())
    val we1 = Input(Bool())
    val dataOut1 = Output(UInt(width.W))
    // Port 2
    val clk2 = Input(Clock())
    val reset2 = Input(Bool())
    val addr2 = Input(addrWidth)
    val dataIn2 = Input(UInt(width.W))
    val en2 = Input(Bool())
    val we2 = Input(Bool())
    val dataOut2 = Output(UInt(width.W))
  })
  println(s"Dual-port Memory Parameters:")
  println(s"  Size: $sizeKB KB")
  println(s"  Width: $width bit")
  println(s"  Addr Width: " + io.addr1.getWidth + " bit")

  val mem = SyncReadMem(sizeKB * 1024, UInt(width.W))
  if (memoryFile.trim().nonEmpty) {
      loadMemoryFromFile(mem, memoryFile)
  }
  // Port 1
  when (io.we1) {
    mem.write(io.addr1, io.dataIn1)
  }
  io.dataOut1 := mem.read(io.addr1, io.en1)

  // Port 2
  withClockAndReset(io.clk2, io.reset2) {
    when (io.we2) {
      mem.write(io.addr2, io.dataIn2)
    }
    io.dataOut2 := mem.read(io.addr2, io.en2)
  }
}

Generates:

=== DualPortRAM ===

   Number of wires:                 48
   Number of wire bits:            370
   Number of public wires:          48
   Number of public wire bits:     370
   Number of memories:               0
   Number of memory bits:            0
   Number of processes:              0
   Number of cells:                 41
     $mem                            1
     LUT4                           20
     TRELLIS_FF                     20

With no DP16KD cells.

The logic is the same between both. I'm using Chisel3 3.4.2.

Found this reference on StackOverflow: https://stackoverflow.com/questions/54789756/vivado-cant-recognize-the-double-port-ram-while-using-syncreadmem

[ekiwi]

Thanks for the report, @carlosedp!

We will need to improve the way that we emit memories to Verilog.
I would be interested in hearing @albert-magyar's opinion about this since it seems like he has been struggling with Vivado failing to infer BRAMs.

[ekiwi]

One thing I noticed is that your DualPortRAM seems to actually describe a 4-ported memory since read and write are not mutually exclusive. So you end up with two read and two write ports.
From the yosys output:

3.24. Executing MEMORY_BRAM pass (mapping $mem cells to block memories).
Processing DualPortRAM.mem:
  Properties: ports=4 bits=32768 rports=2 wports=2 dbits=32 abits=10 words=1024

[ekiwi]

However, if I make them mutually exclusive, the memory is still inferred as having 4 ports. I believe that there might be some problems with correctly inferring combined read/write ports on Chisel memories. @albert-magyar has been working on fixing some of these issue: chipsalliance/firrtl#1821

[albert-magyar]

However, if I make them mutually exclusive, the memory is still inferred as having 4 ports. I believe that there might be some problems with correctly inferring combined read/write ports on Chisel memories. @albert-magyar has been working on fixing some of these issue: chipsalliance/firrtl#1821

This is actually a separate issue that is not related to CHIRRTL memories. Since the beginning of the Chisel3/FIRRTL compiler project, FIRRTL has lowered readwrite ports to separate read and write ports. Therefore, it is unfortunately a known limitation that is is not possible to productively use a true dual-port (TDP) BRAM with native FIRRTL memories.

I have implemented various workarounds for this, and I would be happy to add them to the emitter (i.e., allow certain memories to bypass VerilogMemDelays unmodified), but there are a few associated issues related to the fact that that this emission strategy would likely be better introduced as an option. Currently, the prevailing sentiment has been to rely on memory substitution passes to achieve technology-specific implementations (e.g, --repl-seq-mems), which has weakened the case for a more fully featured Verilog emitter. This was the motivation behind adding the BRAM-canonicalization pass to FireSim.

Ultimately, I think the question is whether there is sufficient buy-in for adding special cases (not necessarily specific to BRAM use) to the emitter for synchronous-read memories vs. the status quo of having users rely on --repl-seq-mems et al. The case of mutually exclusive read/write enables can currently be handled successfully with that flow.

[ekiwi]

@carlosedp : Do you have any example Verilog code of a 2 R/W port memory that gets successfully mapped to a BRAM by yosys? I am having trouble generating such an example. All I found was this yosys issue which seems to indicate that maybe inferring combined R/W ports might not actually work: YosysHQ/yosys#1959

[ekiwi]

There is also this comment on a yosys issue: YosysHQ/yosys#1802 (comment)

true dual port memories with two read/write ports are not supported regardless of clocking

[carlosedp]

Yep, also this comment from David: YosysHQ/yosys#1101 (comment)

It hink it's a Yosys issue and not Chisel.

[jackkoenig]

Thanks for the follow up @carlosedp, should we close this issue?

[carlosedp]

Ah yes, sure.
Thanks @jackkoenig.