[Dedup][FIRRTL] SRAM modules not deduped when using memory macro replacement
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
# 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:
```scala
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...
```scala
//> 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:
```verilog
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:
```verilog
// 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.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Reproduce the issue with the supplied Chisel design and the firtool path, then inspect the generated Verilog alongside metadata/seq_mems.json and foo.conf. The work is done when equivalent SRAM wrapper modules are deduplicated while the shared mem_ext replacement and hierarchy metadata remain correct.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100