llvm / llvm/circt

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

Open
#6,445 3 comments 0 reactions 0 assignees View on GitHub

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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.