Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the SYNTH_VERILOG_SURGERY feature, which allows a custom script to copy and edit Verilog source files prior to synthesis. The build system and synthesis scripts have been updated to read from the resulting SYNTH_VERILOG_FILES instead of the original VERILOG_FILES. Feedback on the changes highlights two potential Makefile issues: a dependency resolution failure if SYNTH_VERILOG_SURGERY starts with a system command like python3, and a shell syntax error in the validation loop if SYNTH_VERILOG_FILES is empty.
4b787d5 to
5434a5e
Compare
Surgery at home: SYNTH_VERILOG_SURGERY names a script that copies VERILOG_FILES and edits the copies before synthesis reads them, so the source tree stays as upstream has it. Two reasons to edit at synthesis time rather than in the source: - Bringing in upstream stays easy. A local edit to the source has to be carried across every bump of the upstream RTL and every regeneration of a generated core. The script applies it as a patch, as a procedure (find this construct, rewrite it), or both; a procedure can survive RTL changes that make a patch fail to apply. - Some seams are PDK-specific. Whether a register file becomes a module of its own that a generated register file replaces, or stays flops for synthesis to map, depends on which gives the best QoR on that PDK, not on the source code. The same RTL can want the seam on one platform and not another, so it belongs in the platform's flow configuration. A hack, documented as one: nothing checks what it changed. Good for simple RTL, especially generated RTL (firtool, sv2v), whose text is regular enough to edit reliably. Unsuitable for idiomatic SystemVerilog code bases with include files, macros, packages, `bind` and other advanced language features, where an edit to the text cannot see what the code means. It runs where the RTL is read (synth_verilog_files in util.tcl, called by read_design_sources and by the SYNTH_USE_SYN frontend), with SYNTH_VERILOG_SURGERY_ARGS, then `--out-dir $(OBJECTS_DIR)/surgery --` and the VERILOG_FILES (by PYTHON_EXE when it ends in .py). It writes one copy per source under the same file name, and synthesis reads the copies. They are kept for inspection but are no make target and no build output: the Makefile and variables.mk are unchanged. A failing script, or a copy it did not write, stops synthesis. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
5434a5e to
c3a7c6c
Compare
🔍 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. |
|
A hack as stated. PDK specific seams are generally handled by having module whose implementation varies by PDK (eg clock gates). It doesn't feel generally applicable enough to be worth the complexity. |
The variable's documentation and the util.tcl comment now say what the step is for: vendored RTL stays byte-identical to upstream and the OpenROAD- and PDK-specific changes live in one script next to the design's config.mk. A diff of a copy against its original shows what the surgery changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
Reopening with a reframe, after a survey of The flow already does a form of this in yosys: AUTO_MEMORIES recognises a memory and makes a module of it. Surgery extends that to patterns yosys cannot see, such as coralnpu's register file, which firtool generated as 31 separate registers behind and-or selects. The description has the survey table, with the commit for each edit; #4653 (coralnpu) is the first user. |
Carries The-OpenROAD-Project/OpenROAD-flow-scripts#4649 as ORFS patch 0094: SYNTH_VERILOG_SURGERY names a script that copies VERILOG_FILES and edits the copies; synthesis reads the copies, so vendored RTL stays as upstream has it. SYNTH_VERILOG_SURGERY_ARGS passes it arguments. A design's config.mk can name the script: config_mk_parser stages it as a source, and the design DSL exports .py files so the label resolves. test/verilog_surgery synthesizes a fixture whose script inverts an assignment the vendored RTL leaves plain; the netlist has the inverter only if synthesis read the copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Carries The-OpenROAD-Project/OpenROAD-flow-scripts#4649 as ORFS patch 0094: SYNTH_VERILOG_SURGERY names a script that copies VERILOG_FILES and edits the copies; synthesis reads the copies, so vendored RTL stays as upstream has it. SYNTH_VERILOG_SURGERY_ARGS passes it arguments. A design's config.mk can name the script: config_mk_parser stages it as a source, and the design DSL exports .py files so the label resolves. test/verilog_surgery synthesizes a fixture whose script inverts an assignment the vendored RTL leaves plain; the netlist has the inverter only if synthesis read the copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@maliberty Another surgery use-case... a stop-gap fix for sv-elab povik/sv-elab#395 |
|
@maliberty TL;DR: editing the vendored copy in git is not the same as surgery. With surgery, the vendored RTL stays as upstream has it, and each PDK's edit lives in that platform's script. In git, the edit becomes code in the vendored tree that only one PDK uses: coralnpu's pr-merge is just an outage: the OpenROAD build timed out downloading bliss from users.aalto.fi, and pr-head is green. Approve with auto-merge on and the re-run will merge it. |
|
@maliberty 🥹 ? |
Getting third-party RTL through OpenROAD on a given PDK takes surgery: a memory becomes a macro, a clock gate becomes the platform's, a synopsys pragma becomes an attribute the frontend reads, a simulation construct goes. ORFS already does this on three in five of its designs, 19 of the 32 vendored RTL trees in
flow/designs/src(survey below), by editing the vendored copy.The flow already does surgery itself, in yosys: AUTO_MEMORIES' pre-synthesis pass recognises a memory in the RTL, an array with read and write ports, and a memory that is a module of its own becomes a generated macro. That works where the RTL already looks like a memory. Where it does not, the step that recognises the pattern and introduces the module has been an edit to the vendored copy.
SYNTH_VERILOG_SURGERYputs that step on the books, as a concern of its own. A script next to the design'sconfig.mkedits copies ofVERILOG_FILES, and synthesis reads the copies. The vendored tree stays byte-identical to upstream, so:$(OBJECTS_DIR)/surgery/under the original file names, so a diff against the original shows exactly what changed;A module whose implementation varies by PDK, like the clock gate, is the tool where upstream already has that boundary; surgery is for where it does not yet.
Make and Bazel. make needs this hook. In Bazel, design-specific surgery is a genrule. This PR starts with design-specific surgery; once a few designs use it, a second survey can show whether a surgery recurs often enough to belong in a library both builds use.
Scope. Best on generated or regular RTL (firtool, sv2v, netlists). A text edit cannot see the meaning of include files, macros, packages or
bind.How it runs. As the RTL is read (canonicalization, memory extraction, and the
SYNTH_USE_SYNfrontend), the script runs withSYNTH_VERILOG_SURGERY_ARGS, then--out-dir $(OBJECTS_DIR)/surgery --and theVERILOG_FILES(throughPYTHON_EXEwhen it ends in.py). It writes one copy per source under the same file name, and synthesis reads the copies. The copies are no make target and no build output: the Makefile andvariables.mkare unchanged. A failing script, or a copy it did not write, stops synthesis.Example: #4653, coralnpu, with the RTL snapshot unchanged. Its integer register file is register-file inference at home: firtool flattened the
Vecof registers into 31 scalars,regfile_1toregfile_31; each of the 8 read ports is a generated and-or select over them, and the 6 write ports write them in onealwaysblock. There is no array, so yosys sees no memory. The surgery recognises that generated pattern, moves the words intoRegfile_rfand rewrites the bypass as a write-through read on its ports, and AUTO_MEMORIES_REGFILES replacesRegfile_rfwith a generated register file. The TCM SRAMs are the same move one step earlier: yosys sees their arrays, but inside a wrapper, so surgery gives each a memory module of firtool's convention, write mask included, for AUTO_MEMORIES to convert. A third surgery leaves out firtool's verification layers. The integration test, with simulation of each module before and after surgery, is The-OpenROAD-Project/bazel-orfs#1187.Survey: the edits to vendored RTL in
flow/designs/src, and the surgery that does the sameEvery edit to upstream text below can be a surgery step. Files ORFS adds next to the RTL (a new top,
macros.v, stubs) and the generation that produced a netlist (sv2v, firtool, pickling) are not surgery and stay as they are. Unedited: uart, ibex_sv, picorv32 and tpu are byte-identical to upstream; microwatt, tinyRocket and the four black_parrot variants are generated and taken as given; chi_async_bridge (rewritten from XiangShan's Chisel) and mock-alu are written in ORFS; mempool_group differs from its source only by deleted comment lines.fakeram7_*dmem.vas fourfakeram7_256x32with bank selectifdef USE_ASAP7fakeram branch inSram_*x128, in place of the TSMC/GF22 branchesmacros.vtsmc65lp_*SRAM instances renamedmem_*(* blackbox *), parameter as localparam, for hardened blockssynopsys full_case/parallel_casecomments as attributessynopsys parallel_casecomments commented outregdeclarations for slang$stopcommented outtranslate_offasifdef SIMULATIONOPENROAD_CLKGATEClockGateICG branches left outprim_clock_gatinglatch asifdef CG_LATCH(* keep_hierarchy *)🤖 Generated with Claude Code