Repository navigation
Conversation
The snapshot is synthesized as it is; what was adapted by editing it, or left to chance, is now the flow's configuration: - The clock was 10.00 in a platform whose unit is the picosecond. The SDC sources the platform's constraints.sdc, with set_max_delay IO budgets, at 1,900 ps, just under the ~1,955 ps the flow reaches; the test enable is held at 0 and the asynchronous reset is not timed. - `-D USE_ASAP7` swapped the TCM SRAMs onto fakeram_512x128 and fakeram_2048x128, which have no write mask: the wrapper computed the byte mask and connected it nowhere, so a masked write wrote all 16 bytes. surgery.py (SYNTH_VERILOG_SURGERY) turns each SRAM wrapper into an instance of a firtool read-write memory module, and AUTO_MEMORIES generates a macro for each, mask and all. - firtool's verification layers are concatenated into the snapshot: 61 binds of *_Verification_Assert into the core. A layer is opt-in; surgery.py removes the verification/ files, which is firtool's output without layers enabled. - The kept hierarchy was whatever exceeded SYNTH_MINIMUM_KEEP_SIZE; it is coralnpu's units by name, with OPENROAD_HIERARCHICAL=1. Each edit is recognised by its text, token for token: a snapshot that is not the one surgery.py knows stops the build. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
coralnpu's Regfile has 32 words of 32 bits with 8 read and 6 write ports, and keeps them as 31 separate registers among a scoreboard, read bypasses and registered read outputs: no array for yosys to infer and no module of its own. surgery.py --regfile checks every read select, write decode, bypass and register statement against the shape firtool wrote, and moves the storage into Regfile_rf: x0 reading zero, eight combinational reads, six writes ORed when two write one word, every word reset to zero while reset is high. Regfile_rf.regfile has AUTO_MEMORIES_REGFILES generate it. Each read port's bypass, a select by read address over every word's decoded write data, becomes a write-through read: the read address compared with each write port's, the hits' data ORed. It is the same function on the register file's ports alone, and leaves the per-word write decode, which the generated register file does inside, with no reader. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
There was a problem hiding this comment.
Code Review
This pull request introduces configuration and surgery scripts for the coralnpu design, including a new register file specification (Regfile_rf.regfile) and a Python script (surgery.py) to post-process the generated Verilog by stripping verification layers, rewriting SRAM wrappers, and optionally restructuring the register file. Feedback on the changes highlights a critical indexing bug in the simulation-only memory initialization task within surgery.py, along with recommendations to ensure the output directory is created before writing and to specify UTF-8 encoding when reading and writing files.
| MARKER = re.compile(r'^// ----- 8< ----- FILE "([^"]+)" ----- 8< -----$', re.M) | ||
|
|
||
|
|
||
| SRAM_TEMPLATE = "module Sram_{D}x128( input clock, input enable, input write, input [{AM1}:0] addr, input [127:0] wdata, input [15:0] wmask, output [127:0] rdata ); `ifdef USE_ASAP7 wire [127:0] nwmask; genvar i; generate for (i = 0; i < 16; i++) begin assign nwmask[8*i +: 8] = {8{wmask[i]}}; end endgenerate fakeram_{D}x128 u_asap7_sram ( .rd_out(rdata), .addr_in(addr), .wd_in(wdata), .we_in(write), .ce_in(enable), .clk(clock) ); `else reg [127:0] mem [0:{DM1}]; reg [{AM1}:0] raddr; assign rdata = mem[raddr]; `ifndef SYNTHESIS task randomMemoryAll; for (int i = 0; i < 128; i++) begin for (int j = 0; j < {D}; j++) begin mem[i][j] = $random; end end endtask initial begin randomMemoryAll; end `endif always @(posedge clock) begin for (int i = 0; i < 16; i++) begin if (enable & write & wmask[i]) begin mem[addr][i*8 +: 8] <= wdata[8*i +: 8]; end end if (enable & ~write) begin raddr <= addr; end end `endif endmodule" |
There was a problem hiding this comment.
There is a critical indexing bug in the simulation-only randomMemoryAll task within SRAM_TEMPLATE.\n\nIn the nested loops:\n- i loops from 0 to 127 (intended for bits, but used as the first index mem[i], which represents words). This means only the first 128 words of the memory are initialized.\n- j loops from 0 to {D} (depth, e.g., 512 or 2048, used as the second index mem[i][j], which represents bits). Since each word is only 128 bits wide, any access where j >= 128 is an out-of-bounds bit select.\n\nTo fix this and make the initialization correct and robust, we can loop over the {D} words and assign a concatenated 128-bit random value using four $random calls.
| SRAM_TEMPLATE = "module Sram_{D}x128( input clock, input enable, input write, input [{AM1}:0] addr, input [127:0] wdata, input [15:0] wmask, output [127:0] rdata ); `ifdef USE_ASAP7 wire [127:0] nwmask; genvar i; generate for (i = 0; i < 16; i++) begin assign nwmask[8*i +: 8] = {8{wmask[i]}}; end endgenerate fakeram_{D}x128 u_asap7_sram ( .rd_out(rdata), .addr_in(addr), .wd_in(wdata), .we_in(write), .ce_in(enable), .clk(clock) ); `else reg [127:0] mem [0:{DM1}]; reg [{AM1}:0] raddr; assign rdata = mem[raddr]; `ifndef SYNTHESIS task randomMemoryAll; for (int i = 0; i < 128; i++) begin for (int j = 0; j < {D}; j++) begin mem[i][j] = $random; end end endtask initial begin randomMemoryAll; end `endif always @(posedge clock) begin for (int i = 0; i < 16; i++) begin if (enable & write & wmask[i]) begin mem[addr][i*8 +: 8] <= wdata[8*i +: 8]; end end if (enable & ~write) begin raddr <= addr; end end `endif endmodule" | |
| SRAM_TEMPLATE = "module Sram_{D}x128( input clock, input enable, input write, input [{AM1}:0] addr, input [127:0] wdata, input [15:0] wmask, output [127:0] rdata ); `ifdef USE_ASAP7 wire [127:0] nwmask; genvar i; generate for (i = 0; i < 16; i++) begin assign nwmask[8*i +: 8] = {8{wmask[i]}}; end endgenerate fakeram_{D}x128 u_asap7_sram ( .rd_out(rdata), .addr_in(addr), .wd_in(wdata), .we_in(write), .ce_in(enable), .clk(clock) ); `else reg [127:0] mem [0:{DM1}]; reg [{AM1}:0] raddr; assign rdata = mem[raddr]; `ifndef SYNTHESIS task randomMemoryAll; for (int i = 0; i < {D}; i++) begin mem[i] = {$random, $random, $random, $random}; end endtask initial begin randomMemoryAll; end `endif always @(posedge clock) begin for (int i = 0; i < 16; i++) begin if (enable & write & wmask[i]) begin mem[addr][i*8 +: 8] <= wdata[8*i +: 8]; end end if (enable & ~write) begin raddr <= addr; end end `endif endmodule" |
| ) | ||
| p.add_argument("files", nargs="+") | ||
| a = p.parse_args() | ||
| done = 0 |
There was a problem hiding this comment.
The output directory a.out_dir is not guaranteed to exist. If it does not exist, attempting to write files to it will raise a FileNotFoundError. It is safer to ensure the directory exists by calling os.makedirs(a.out_dir, exist_ok=True) before entering the file processing loop.
os.makedirs(a.out_dir, exist_ok=True)
done = 0| a = p.parse_args() | ||
| done = 0 | ||
| for path in a.files: | ||
| with open(path) as f: |
There was a problem hiding this comment.
When opening files in Python 3, it is highly recommended to specify the encoding (e.g., encoding="utf-8") to ensure platform-independent behavior and avoid potential UnicodeDecodeError on systems where the default encoding is not UTF-8.
| with open(path) as f: | |
| with open(path, encoding="utf-8") as f: |
| % (os.path.basename(path), len(removed)) | ||
| ) | ||
| done += 1 | ||
| with open(os.path.join(a.out_dir, os.path.basename(path)), "w") as f: |
There was a problem hiding this comment.
|
Re-measured exactly as this PR has it, without the
Period and area hold. Power does not: with the latch-and-AND clock gate the flop build's power nearly doubles (33.0 → 63.3 mW) and the register file's less (35.8 → 45.4 mW), so the power figure depends on how that one clock gate is modelled. The description now carries these numbers, with that caveat. 🤖 Generated with Claude Code |
🔍 QoR checkMetrics reflect the PR merge build — i.e. what will land on the target branch. Commit 62 design(s) checked — 0 with regression(s), 0 without a comparable baseline. |
asap7/coralnpu and asap7/coralnpu-regfile are in The-OpenROAD-Project/OpenROAD-flow-scripts#4653, its files the same as ORFS patches 0102 and 0103 but for black's line breaks in surgery.py and a space in constraint.sdc. The ladder stops carrying them: 0102 and 0103 go, 0104 keeps only swerv_wrapper-regfile-mux, and coralnpu's rows, its synthesis, surgery and simulation tests leave test/regfile/ladder. designs_by_directory_test checks the swerv mux variant against its base. The design DSL stops exporting .ys files: no design in the series names a SYNTH_POST_HIERARCHY_SCRIPTS script from another package since coralnpu took #4653's form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
asap7/coralnpu and asap7/coralnpu-regfile are in The-OpenROAD-Project/OpenROAD-flow-scripts#4653, its files the same as ORFS patches 0102 and 0103 but for black's line breaks in surgery.py and a space in constraint.sdc. The ladder stops carrying them: 0102 and 0103 go, 0104 keeps only swerv_wrapper-regfile-mux, and coralnpu's rows, its synthesis, surgery and simulation tests leave test/regfile/ladder. designs_by_directory_test checks the swerv mux variant against its base. The design DSL stops exporting .ys files: no design in the series names a SYNTH_POST_HIERARCHY_SCRIPTS script from another package since coralnpu took #4653's form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
Where the flop baseline's period goes, and why the register file is the lever I took this PR's flop baseline to global route and asked where its reg2reg period goes: OpenROAD 22b7c537, ORFS ef42174 with this PR applied, built with bazel-orfs. Here it reaches 2,031 ps against the 1,900 ps clock. The description reports 1,976 ps; the OpenROAD commit and flow differ. One path family sets the period. All of the worst 200 reg2reg paths start at the register file's read data ( It is logic, not placement. Mean over those 200 paths:
The modules on the path are compact. Measured by the radius holding half of each module's cell area, against a disc at the design's density, the LSU is 0.83× and the register file 1.18×. A perfect placement would recover at most the roughly 550 ps of repeaters and wire. It is a mass, not one path. Removing the worst launching register 40 times ( This is the cycle the design chose. CoralNPU is an in-order three-stage pipeline: fetch, decode/dispatch, execute/writeback, up to 4 instructions per cycle (microarch.md). Operands are registered at the dispatch → execute boundary. Then one execute cycle does the rest: the register file adds the immediate ( Compared with the literature, the following appears to be left as an exercise for the reader:
What silicon exists. Synaptics' Astra SL2610 (12 nm) is the first production implementation of Coral NPU, "a tightly-coupled CPU optimized for Synaptics' PPA requirements", with the scalar core only; the vector and matrix units are still in development. Google offers the design as IP, with VeriSilicon productising it, and states a target of 800 MHz (1,250 ps) at 22 nm. This snapshot is the open reference design that production silicon is derived from, not a taped-out netlist; how Synaptics' implementation handled the points above is not public. What follows for this PR. The period is set by the register file's read and address logic feeding the LSU, in a cycle the design intends to close as written. Of the exercises above, the register file is the one a flow can take on without changing the RTL's behaviour, and building it as a generated array is what the second commit does (1,976 → 1,900 ps in the description). The remaining gap to the design's own target, 1,250 ps at 22 nm against 2,031 ps here on asap7, a faster node, is either implementation (how synthesis maps this one cycle) or this snapshot's configuration (instruction lanes, FPU and vector enabled), which may be heavier than the 800 MHz one. I have not separated the two. Sources: CoralNPU Chisel and docs, Google Developers Blog, EE Times, VeriSilicon, Ibex ID/EX, CV32E40P LSU. |
|
Notes for later: getting the most testing value out of CoralNPU with one timing budget These are notes on what to investigate later, written up here for now so they sit next to the design they're about. Nothing below has been tried in this PR yet: the budget, its value and its effect on WNS are proposals to check, not results. CoralNPU is a valuable addition to ORFS's asap7 set. It's an open RISC-V design with production silicon behind it (Synaptics' Astra SL2610), it's hierarchical, with this PR its SRAMs are generated macros and its register file can be a generated array, and it runs synthesis to global route in about an hour. Few designs exercise memory generation, macro placement and timing closure together at a turnaround a flow developer can iterate on. Where its WNS comes from today. Taken to global route with this PR's flop baseline, its reg2reg period is about 2,031 ps (OpenROAD 22b7c537, ORFS ef42174). That number comes from one family: the register file's read data into the LSU's input queue ( Why that family is special. CoralNPU's pipeline does the register file read, the address add and the LSU enqueue in one execute cycle (microarch.md), with an 8-read, 6-write register file written as flops. The literature resolves this cycle in one of two ways. Classic pipelines register the effective address before the memory side (the EX/MEM latch of Hennessy & Patterson's five-stage pipeline, as in Rocket and CV32E40P). Register files with this many ports are built as a structure of their own, banked or custom arrays (Rixner et al., HPCA 2000), rather than flops behind mux trees. The open RTL leaves that step to the implementer. An implementation for silicon would most likely take one of those routes. Synaptics describes its version as "optimized for Synaptics' PPA requirements"; the details aren't public. The idea: time that family against its own budget. Without touching the RTL, # The register file's read data into the LSU's input queue: one execute
# cycle in the open RTL, a cycle an implementation for silicon resolves
# (an address register, or a register file built as an array). Timed
# against its own budget, so the rest of the design sets this test
# case's WNS; the paths stay timed and repaired, and a regression shows
# as negative slack here.
set rf_read [get_cells core.score/regfile/io_readData_*]
set lsu_q [get_pins core.score/lsu/ctrl.mem_*/D]
if { [llength $rf_read] == 0 || [llength $lsu_q] == 0 } {
error "constraint.sdc: the register-file -> LSU family matched nothing"
}
set_max_delay 2100 -from $rf_read -to $lsu_qWhat it should give, to be confirmed:
Two numbers per run: WNS for the design against the clock, and the register-file family's slack against its budget. Together they would say how the flow does on the design as a whole, and how the one-cycle read-to-LSU path does on its own. With the generated register file. This PR's second commit takes one of the literature's routes for real: To investigate: whether the budget matches the paths it should and only those; what WNS the rest of the design reports with it; whether that number responds to floorplan and placement changes; and what budget value keeps the family met today while still catching regressions. |
|
@maliberty TL;DR, no action needed now:
🤖 Generated with Claude Code |
The encoding is no longer the platform default's, as the review asked. The docstring no longer calls the surgery a hack: it is the step that puts the build's edits to the snapshot on the books. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The open RTL resolves a register-file read, an ALU result forwarded through the register file, dispatch's issue decision and the load/store unit's enqueue in one cycle. surgery.py --address-register registers LsuV1's inputs (each lane's request and valid, the effective addresses and the store data), the EX/MEM register of the classic five-stage pipeline; LsuV1 builds and enqueues its uops from it a cycle later. io_req_N_ready keeps its thresholds, applied to the entries the queue holds plus those the register will enqueue, and io_active includes the register, so a fence waits for it. Loads and stores take one cycle longer. coralnpu-regfile-exmem combines it with coralnpu-regfile. Each has its own clock just under what it reaches, as coralnpu's is: 1,700 ps for coralnpu-exmem, which reaches 1,701 ps at global route, and 1,650 ps for coralnpu-regfile-exmem, which meets 1,700 ps. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ader What the four coralnpu designs build and their results at global route; where the RTL comes from and what is missing (a recipe that regenerates it from a pinned upstream commit); which surgery closes the gap between upstream and an idiomatic ORFS build, and which stands in for a fresh snapshot; the EX/MEM register with a diagram, what is public about how the silicon met the same cycle, and what is not solved here. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Results: with the EX/MEM register, coralnpu goes from 2,031 ps to 1,701 ps at global route (1,700 ps clock); with the generated register file too, it meets 1,700 ps. The worst path moves to register file → CSR Correction to my earlier analysis: the queue is 🤖 Generated with Claude Code |
| # concatenated in: 61 binds of *_Verification_Assert into the core. A | ||
| # layer is opt-in, so a synthesis build leaves them out; surgery.py | ||
| # removes them, as firtool's output without layers enabled would be. | ||
| export SYNTH_VERILOG_SURGERY = $(DESIGN_HOME)/$(PLATFORM)/coralnpu/surgery.py |
There was a problem hiding this comment.
Where is this file? I don't see it in the PR
There was a problem hiding this comment.
This is a draft where I keep notes, the file is still in a patch in bazel-orfs for now.
Once all fhe auto memories land in ORFS, I will clean this up if you want it, otherwise it has been useful for my testing when working on auto memories.
A showcase of
AUTO_MEMORIES_REGFILESandSYNTH_VERILOG_SURGERYon coralnpu, whose integer register file has 8 read and 6 write ports. The RTL snapshot is unchanged: the adaptation is a config, an SDC, a surgery script and a register-file spec.flow/designs/asap7/coralnpu/README.mddescribes all four designs.Surgery: an exercise left to the reader, solved. In the snapshot, one execute cycle reads the register file, forwards an ALU result through it, lets dispatch decide which lanes issue, and has the load/store unit build its micro-ops and enqueue them. That one family of paths sets the period.
coralnpu-exmemadds the textbook EX/MEM register on the load/store unit's inputs (Hennessy & Patterson's five-stage pipeline; Rocket and CV32E40P do the same). It is a stand-in for a fresh snapshot: upstream coralnpu has since reworked this cycle in Chisel (43d2030).coralnpu(flops)coralnpu-regfilecoralnpu-exmemcoralnpu-regfile-exmemOpenROAD 22b7c537 and ORFS ef42174 with bazel-orfs's carried patches, this PR among them. Each design's clock sits just under what it reaches, so repair has a target and the WNS is slightly negative; 1,650 ps for
coralnpu-regfile-exmemis a guess under the 1,700 ps it meets. In the flop build, nearly all of the CTS and global route time isrepair_timingchasing the one family of paths.The commits.
Regfilekeeps its words as separate registers among a scoreboard and bypasses. Surgery moves the storage into a module the generator replaces, and rewrites the bypass as a write-through read on the register file's ports.surgery.pyreads and writes UTF-8 (review).surgery.py --address-registerregistersLsuV1's inputs (each lane's request and valid, the effective addresses, the store data).io_req_N_readykeeps its thresholds, applied to the queue's count plus the entries the register will enqueue;io_activeincludes the register, so a fence waits for it. Loads and stores take one cycle longer.Correctness, in bazel-orfs (carried there as patches):
LsuV1as in the snapshot against the EX/MEM one under the same random requests, up to four lanes a cycle with the buses stalling at random: every bus transaction, writeback, flush and fault identical, channel by channel, for three seeds of 20,000 requests; a negative control without the ready adjustment loses requests and must differ, and does; the surgery changesLsuV1and no other module.Power is not in the table: coralnpu's top-level
ClockGatesynthesizes as a latch and an AND (upstream instantiates a library ICG per process; this PR leaves it as is), and the power figure depends on how that one clock gate is modelled (see the earlier comment).Depends on various and sundry AUTO_MEMORIES PRs and on The-OpenROAD-Project/OpenROAD#11669: a draft until they are merged.
🤖 Generated with Claude Code