[Dedup][FIRRTL] SRAM modules not deduped when using memory macro replacement

[tymcauley]

Context

Platform: macOS 14.1.1 Architecture: arm64 circt version: Firtool 1.58.0 release, also f124fb041

Description

When generating a design from Chisel with arrays of SRAM instances, I notice that the SRAM instances are placed inside of modules that aren't deduped. Here's a small Chisel design to reproduce the issue with an array of SRAMs, each with a single read-write port:

class Foo extends Module {
  val numMems    = 2
  val numEntries = 16
  val dType      = UInt(8.W)

  val addr   = IO(Vec(numMems, Input(UInt(log2Ceil(numEntries).W))))
  val inData = IO(Vec(numMems, Input(dType)))
  val out    = IO(Vec(numMems, Output(dType)))
  val wen    = IO(Vec(numMems, Input(Bool())))

  for (i <- 0 until numMems) {
    val mem = SyncReadMem(numEntries, dType)

    val rwPort = mem(addr(i))
    out(i) := DontCare
    when (wen(i)) {
      rwPort := inData(i)
    }.otherwise {
      out(i) := rwPort
    }
  }
}

If you run that through a Chisel 3.6.0 flow, using the SFC...

//> using scala "2.13.10"
//> using lib "edu.berkeley.cs::chisel3::3.6.0"
//> using plugin "edu.berkeley.cs:::chisel3-plugin::3.6.0"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
import chisel3.util.log2Ceil

class Foo extends Module {
  // Insert design from above
}

object Main extends App {
  val useFirtool = false
  if (!useFirtool) {
    val pretty = Array(
      "--infer-rw",
      "--repl-seq-mem",
      "-c:Foo:-o:foo.conf",
      "--emission-options",
      "disableMemRandomization,disableRegisterRandomization",
    )
    println(getVerilogString(new Foo, pretty))
  } else {
    println(
      circt.stage.ChiselStage.emitSystemVerilog(
        gen = new Foo,
        firtoolOpts = Array(
          "-disable-all-randomization",
          "-strip-debug-info",
          "-repl-seq-mem",
          "-repl-seq-mem-file=foo.conf",
        ),
      )
    )
  }
}

Then you get this:

module Foo(
  input        clock,
  input        reset,
  input  [3:0] addr_0,
  input  [3:0] addr_1,
  input  [7:0] inData_0,
  input  [7:0] inData_1,
  output [7:0] out_0,
  output [7:0] out_1,
  input        wen_0,
  input        wen_1
);
  wire [3:0] mem_RW0_addr;
  wire  mem_RW0_clk;
  wire  mem_RW0_wmode;
  wire [7:0] mem_RW0_wdata;
  wire [7:0] mem_RW0_rdata;
  wire [3:0] mem_1_RW0_addr;
  wire  mem_1_RW0_clk;
  wire  mem_1_RW0_wmode;
  wire [7:0] mem_1_RW0_wdata;
  wire [7:0] mem_1_RW0_rdata;
  mem mem (
    .RW0_addr(mem_RW0_addr),
    .RW0_clk(mem_RW0_clk),
    .RW0_wmode(mem_RW0_wmode),
    .RW0_wdata(mem_RW0_wdata),
    .RW0_rdata(mem_RW0_rdata)
  );
  mem mem_1 (
    .RW0_addr(mem_1_RW0_addr),
    .RW0_clk(mem_1_RW0_clk),
    .RW0_wmode(mem_1_RW0_wmode),
    .RW0_wdata(mem_1_RW0_wdata),
    .RW0_rdata(mem_1_RW0_rdata)
  );
  assign out_0 = mem_RW0_rdata;
  assign out_1 = mem_1_RW0_rdata;
  assign mem_RW0_addr = addr_0;
  assign mem_RW0_clk = clock;
  assign mem_RW0_wmode = wen_0;
  assign mem_RW0_wdata = inData_0;
  assign mem_1_RW0_addr = addr_1;
  assign mem_1_RW0_clk = clock;
  assign mem_1_RW0_wmode = wen_1;
  assign mem_1_RW0_wdata = inData_1;
endmodule
module mem(
  input  [3:0] RW0_addr,
  input        RW0_clk,
  input        RW0_wmode,
  input  [7:0] RW0_wdata,
  output [7:0] RW0_rdata
);
  wire [3:0] mem_ext_RW0_addr;
  wire  mem_ext_RW0_en;
  wire  mem_ext_RW0_clk;
  wire  mem_ext_RW0_wmode;
  wire [7:0] mem_ext_RW0_wdata;
  wire [7:0] mem_ext_RW0_rdata;
  mem_ext mem_ext (
    .RW0_addr(mem_ext_RW0_addr),
    .RW0_en(mem_ext_RW0_en),
    .RW0_clk(mem_ext_RW0_clk),
    .RW0_wmode(mem_ext_RW0_wmode),
    .RW0_wdata(mem_ext_RW0_wdata),
    .RW0_rdata(mem_ext_RW0_rdata)
  );
  assign mem_ext_RW0_clk = RW0_clk;
  assign mem_ext_RW0_en = 1'h1;
  assign mem_ext_RW0_addr = RW0_addr;
  assign RW0_rdata = mem_ext_RW0_rdata;
  assign mem_ext_RW0_wmode = RW0_wmode;
  assign mem_ext_RW0_wdata = RW0_wdata;
endmodule

Notice that there's only one mem module definition, which wraps around mem_ext.

If you run this through firtool (change that useFirtool flag from false to true), you get this:

// Generated by CIRCT unknown git version
module Foo(
  input        clock,
               reset,
  input  [3:0] addr_0,
               addr_1,
  input  [7:0] inData_0,
               inData_1,
  output [7:0] out_0,
               out_1,
  input        wen_0,
               wen_1
);

  mem mem (
    .RW0_addr  (addr_0),
    .RW0_clk   (clock),
    .RW0_wmode (wen_0),
    .RW0_wdata (inData_0),
    .RW0_rdata (out_0)
  );
  mem_1 mem_1 (
    .RW0_addr  (addr_1),
    .RW0_clk   (clock),
    .RW0_wmode (wen_1),
    .RW0_wdata (inData_1),
    .RW0_rdata (out_1)
  );
endmodule

module mem(
  input  [3:0] RW0_addr,
  input        RW0_clk,
               RW0_wmode,
  input  [7:0] RW0_wdata,
  output [7:0] RW0_rdata
);

  mem_ext mem_ext (
    .RW0_addr  (RW0_addr),
    .RW0_en    (1'h1),
    .RW0_clk   (RW0_clk),
    .RW0_wmode (RW0_wmode),
    .RW0_wdata (RW0_wdata),
    .RW0_rdata (RW0_rdata)
  );
endmodule

// external module mem_ext

module mem_1(
  input  [3:0] RW0_addr,
  input        RW0_clk,
               RW0_wmode,
  input  [7:0] RW0_wdata,
  output [7:0] RW0_rdata
);

  mem_ext mem_ext (
    .RW0_addr  (RW0_addr),
    .RW0_en    (1'h1),
    .RW0_clk   (RW0_clk),
    .RW0_wmode (RW0_wmode),
    .RW0_wdata (RW0_wdata),
    .RW0_rdata (RW0_rdata)
  );
endmodule

// ----- 8< ----- FILE "metadata/seq_mems.json" ----- 8< -----

[
  {
    "module_name": "mem_ext",
    "depth": 16,
    "width": 8,
    "masked": false,
    "read": 0,
    "write": 0,
    "readwrite": 1,
    "extra_ports": [],
    "hierarchy": [
      "Foo.mem_1.mem_ext",
      "Foo.mem.mem_ext"
    ]
  }
]

// ----- 8< ----- FILE "foo.conf" ----- 8< -----

name mem_ext depth 16 width 8 ports rw

While there's only one mem_ext definition, the wrapper modules mem/mem_1 aren't deduplicated.

[tymcauley]

I realize this might be a bit easier to reproduce by just using the .fir output. Here's the FIRRTL from Chisel 5.1.0:

FIRRTL version 2.0.0
circuit Foo :
  module Foo :
    input clock : Clock
    input reset : UInt<1>
    input addr : UInt<4>[2]
    input inData : UInt<8>[2]
    output out : UInt<8>[2]
    input wen : UInt<1>[2]

    smem mem : UInt<8> [16]
    infer mport rwPort = mem[addr[0]], clock
    out[0] is invalid
    when wen[0] :
      rwPort <= inData[0]
    else :
      out[0] <= rwPort
    smem mem_1 : UInt<8> [16]
    infer mport rwPort_1 = mem_1[addr[1]], clock
    out[1] is invalid
    when wen[1] :
      rwPort_1 <= inData[1]
    else :
      out[1] <= rwPort_1

You can reproduce using:

firtool --repl-seq-mem --repl-seq-mem-file=foo.conf Foo.fir

[tymcauley]

Okay, after investigating a bit:

• If you remove the --repl-seq-mem --repl-seq-mem-file=... options, the memory modules will be deduped. In particular, lower-seq-to-sv will merge the two seq.firmem operations.
• However, when including the --repl-seq-mem --repl-seq-mem-file=... options, the firrtl-lower-memory pass will convert the two firrtl.mem operations into two separate, identical firrtl.modules (mem and mem_1), but this is after the dedup pass has already run, so these two module definitions don't get deduped.

[tymcauley]

Might want to re-open this issue after #6719 got reverted in https://github.com/llvm/circt/commit/bea85107e8e42261e9c2e3ed24dd398e7f33b915, waiting on #6830 to be completed before re-marking this as complete.