smem read port always reads address 0

[luismarques]

Type of issue: bug report

I'm having problems with smem memories. Consider this circuit as an example:

circuit Memories :
    module Memories :
        input clock : Clock
        input reset : UInt<1>
        input writeAddr : UInt<2>
        input writeVal : UInt<8>
        input readAddr : UInt<2>
        output readVal : UInt<8>
        readVal is invalid

        smem mem : UInt<8>[3]
        infer mport _T_0 = mem[writeAddr], clock
        _T_0 <= writeVal
        infer mport _T_1 = mem[readAddr], clock
        readVal <= _T_1

When I convert this to Verilog (using firrtl git HEAD, commit ed70957), I get the following (trimmed for clarity):

  reg [1:0] mem__T_1_addr_pipe_0;
  assign mem__T_1_addr = mem__T_1_addr_pipe_0;
  assign mem__T_1_data = mem[mem__T_1_addr]; // @[:Memories.fir@11.8]
  assign readVal = mem__T_1_data; // @[:Memories.fir@15.8]
  wire  _GEN_0;
  assign _GEN_0 = 1'h0;
  always @(posedge clock) begin
    if (_GEN_0) begin
      mem__T_1_addr_pipe_0 <= readAddr;
    end
  end

Because of the assign _GEN_0 = 1'h0, the mem__T_1_addr_pipe_0 is never written to, so the circuit always reads from the memory address 0 (or whatever mem__T_1_addr_pipe_0 is initialized to).

This looks like a bug, but it's hard to say exactly because smem memories are not documented. Any insights into this?

[jackkoenig]

This is most certainly a bug, and so far, I'm only more confused, I generated the following with Chisel:

circuit SMem :
  module SMem :
    input clock : Clock
    input reset : UInt<1>
    output io : {flip raddr : UInt<2>, rdata : UInt<8>, flip waddr : UInt<2>, flip wdata : UInt<8>}
    io.rdata is invalid

    smem mem : UInt<8>[3]
    infer mport _T_15 = mem[io.waddr], clock
    _T_15 <= io.wdata
    infer mport _T_16 = mem[io.raddr], clock
    io.rdata <= _T_16      

The only difference is an aggregate IO, and yet this works:

  reg [1:0] mem__T_16_addr_pipe_0;
  assign mem__T_16_addr = mem__T_16_addr_pipe_0;
  assign mem__T_16_data = mem[mem__T_16_addr];
  wire  _GEN_0;
  assign _GEN_0 = 1'h1;
  always @(posedge clock) begin
    if (_GEN_0) begin
      mem__T_16_addr_pipe_0 <= io_raddr;
    end
  end

[jackkoenig]

Workaround

The issue comes from using a non-aggregate input as a read address. You can work around this by assigning the read address input to an intermediate node or wire and then using that wire as the address for the memory. For example:

 circuit Memories :
    module Memories :
        ...
        input readAddr : UInt<2>
        ...
        smem mem : UInt<8>[3]
        node realReadAddr = readAddr                 ; <- Look here
        infer mport _T_1 = mem[realReadAddr], clock
        readVal <= _T_1

Diagnosis

Possibly related to #840

This appears to be due to logic introduced over two years ago in #203. The difference between my code that works, and the given code that doesn't is the non-working code follows this code path: https://github.com/freechipsproject/firrtl/blob/ceac36d7ce1223078ca47bc097884532faacd7e1/src/main/scala/firrtl/passes/RemoveCHIRRTL.scala#L144

I think this code path is used for R/W port inference. In this case, a read port enable cannot be set to UInt(1) at memory port declaration as it does on the other code path, since we might have a R/W port. Instead, it records the read port address for later. The bug is that the read port enable being driven with UInt(1) is only injected into the AST when the pass comes across the point where the memory read port address is driven. This is fine if the address is a wire, node, or register, but does not work for inputs.

It appears this issue was fixed for aggregate ports by #226 but this looks like a corner case that slipped through that fix. I would also bet that #203 (ie. R/W port inference) does not work with fields of aggregate wires being used as memory read port addresses.

I can patch this error, but I think similar such errors lurk in RemoveCHIRRTL, the real fix is #727

[mehnadnerd]

I ran into this same issue where the address was created by Cat, and was able to use the workaround of having an intermediate wire.
val tableindex = Cat(az_sign, unsignedtableindex) val tableindex2 = Wire(UInt()) tableindex2 := tableindex tr := table.read(tableindex2)

[ekiwi]

Just stumbled over the same bug.

circuit smem :
  module smem :
    input clock : Clock
    input addr : UInt<3>
    output out : UInt<8>

    smem mem : UInt<8>[5]

    read mport r = mem[addr], clock
    out <= r

gets lowered to:

circuit smem :
  module smem :
    input clock : Clock
    input addr : UInt<3>
    output out : UInt<8>
  
    mem mem :
      data-type => UInt<8>
      depth => 5
      read-latency => 1
      write-latency => 1
      reader => r
      read-under-write => undefined
    out <= mem.r.data
    mem.r.addr <= addr
    mem.r.en <= UInt<1>("h0")            ; BUG: read port is always disabled!
    mem.r.clk <= clock

Adding a node fixes that:

circuit smem :
  module smem :
    input clock : Clock
    input addr : UInt<3>
    output out : UInt<8>

    smem mem : UInt<8>[5]

    node issue_913 = addr
    read mport r = mem[issue_913], clock
    out <= r

lowers to

  module smem :
    input clock : Clock
    input addr : UInt<3>
    output out : UInt<8>
  
    mem mem :
      data-type => UInt<8>
      depth => 5
      read-latency => 1
      write-latency => 1
      reader => r
      read-under-write => undefined
    node issue_913 = addr
    out <= mem.r.data
    mem.r.addr <= issue_913
    mem.r.en <= UInt<1>("h1")                ;  read port is always enabled!
    mem.r.clk <= clock

[albert-magyar]

Closed and absorbed into #727