[FIRRTL] smem with read address from port does not work

[jackkoenig]

Consider the following FIRRTL:

FIRRTL version 4.0.0
circuit Top :
  public module Top :
    input clock : Clock
    input raddr : UInt<6>
    input waddr : UInt<6>
    input wdata : UInt<8>
    output rdata : UInt<8>

    smem ram : UInt<8>[64]

    write mport w = ram[waddr], clock
    connect w, wdata

    read mport r = ram[raddr], clock
    connect rdata, r

Compile this with firtool 1.93.1 and you will get:

test.fir:16:5: warning: memory port is never enabled
    read mport r = ram[raddr], clock
    ^
test.fir:16:5: note: see current operation: %r_data, %r_port = chirrtl.memoryport Read %ram {name = "r"} : (!chirrtl.cmemory<uint<8>, 64>) -> (!firrtl.uint<8>, !chirrtl.cmemoryport)
// Generated by CIRCT firtool-1.93.1
module Top(
  input        clock,
  input  [5:0] raddr,
               waddr,
  input  [7:0] wdata,
  output [7:0] rdata
);

  assign rdata = 8'h0;
endmodule

If you simply add a node to use as the read address instead of the input port, it works. That is, just change the read port to:

    node readAddr = raddr
    read mport r = ram[readAddr], clock
    connect rdata, r

Then the emitted Verilog has a memory instantiated as expected.

[seldridge]

Blaming the code that is doing this and looking at what the SFC would do in this situation seems to indicate that this is there for SFC compatibility. The LowerCHIRRTL has specific (old) comments about how enable inference works and that nodes are treated specially. This is also a very, very old SFC issue where this was always the behavior (see: chipsalliance/firrtl#727 (comment)).

Just for completeness, compiling the above with SFC 1.5.6 produces a memory whose read is not enabled:

module Top(
  input        clock,
  input  [5:0] raddr,
  input  [5:0] waddr,
  input  [7:0] wdata,
  output [7:0] rdata
);
  reg [7:0] ram [0:63];
  wire  ram_r_en;
  wire [5:0] ram_r_addr;
  wire [7:0] ram_r_data;
  wire [7:0] ram_w_data;
  wire [5:0] ram_w_addr;
  wire  ram_w_mask;
  wire  ram_w_en;
  reg  ram_r_en_pipe_0;
  reg [5:0] ram_r_addr_pipe_0;
  assign ram_r_en = ram_r_en_pipe_0;
  assign ram_r_addr = ram_r_addr_pipe_0;
  assign ram_r_data = ram[ram_r_addr];
  assign ram_w_data = wdata;
  assign ram_w_addr = waddr;
  assign ram_w_mask = 1'h1;
  assign ram_w_en = 1'h1;
  assign rdata = ram_r_data;
  always @(posedge clock) begin
    if (ram_w_en & ram_w_mask) begin
      ram[ram_w_addr] <= ram_w_data;
    end
    ram_r_en_pipe_0 <= 1'h0;
    if (1'h0) begin
      ram_r_addr_pipe_0 <= raddr;
    end
  end
endmodule

Making this SFC-bug (or feature?) compatible made sense at the time. However, it doesn't make sense now.

What I would like to see is the promote cmem and smem to first-class FIRRTL specification features. Using the port/access features from #1561 would avoid the scoping problems of the original cmem/smem design where the port declarations were accessible outside their defining scopes.