Bazel orfs update#4229
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces diagnostic tools, specifically yosys-check and make-yosys-netlist, to triage Quality of Results (QoR) divergences between Bazel and Make flows. It also updates bazel-orfs with a patch for shared block.mk handling, adds several new flow variables (LIB_MODEL, MIN_CLK_ROUTING_LAYER, SDC_FILE_EXTRA), and refines file exporting in design packages to support cross-package references. Feedback suggests skipping hierarchical designs in yosys-check.sh for consistency with its companion script, expanding the list of exported file extensions in design.bzl to include GDS files, and refactoring duplicated export logic into a private helper function.
Three drive-by suggestions from the bot review: - bazel/yosys-check.sh: refuse BLOCKS= designs up front, mirroring make-yosys-netlist.sh. Same rationale — bazel-orfs's hierarchical block plumbing diverges from make's (sub-block .lef/.lib not staged into the parent synth action), so the per-stage .odb table would not be apples-to-apples. - flow/designs/design.bzl: include gds and gds.gz in _EXPORTED_EXTS so cross-package ADDITIONAL_GDS references resolve. Brings the exported set into line with the _GROUPS["gds"] entry that already filegroups them. - flow/designs/design.bzl: factor the duplicated exports_files() glob in design() and files() into a private _export_design_files() helper. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the Bazel-based OpenROAD flow by disabling lockfile tracking, updating the Yosys version to 0.63, and patching bazel-orfs to support shared block.mk configurations in hierarchical designs. It introduces two new utility scripts, yosys-check and make-yosys-netlist, along with extensive documentation to help triage flow divergences and verify OpenROAD determinism between Bazel and Make. Additionally, new flow variables such as LIB_MODEL and MIN_CLK_ROUTING_LAYER were added, and design files are now exported as individual labels to support cross-package references. Feedback focused on improving the robustness of the new shell scripts when identifying results subdirectories to avoid non-deterministic behavior if multiple variants exist.
|
will re-articulate |
bazel-orfs validates every variable used by a design's config.mk against
flow/scripts/variables.yaml. These three are real ORFS variables already
used by platform configs, Tcl scripts, or recognised by bazel-orfs as
source-typed, but were missing schema declarations, which made the bazel
load phase fail for any design that referenced them:
- LIB_MODEL: selects NLDM vs CCS lib files in
flow/platforms/asap7/config.mk and gates CCS-specific branches in
flow/scripts/load.tcl. Used by asap7/gcd-ccs and asap7/swerv_wrapper.
- MIN_CLK_ROUTING_LAYER: lower bound for clock-net routing in
flow/platforms/{asap7,sky130hd,nangate45,sky130hs}/fastroute.tcl.
Platforms set defaults; designs (asap7/riscv32i-mock-sram fakeram,
sky130hd/ibex) override. No stages: list — floorplan.tcl sources the
platform fastroute.tcl too, mirroring MIN/MAX_ROUTING_LAYER.
- SDC_FILE_EXTRA: per-design extra Tcl hook source'd from a design's
own SDC / io.tcl. bazel-orfs's config_mk_parser already classifies
this as a source-typed (path-label) variable in SOURCE_VARS, so
the value is staged into the sandbox. Used by asap7/mock-cpu.
flow/scripts/variables.json and docs/user/FlowVariables.md regenerated
via flow/scripts/yaml_to_json.py and flow/scripts/generate-variables-docs.py
so the derived artefacts stay in sync.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
These two variables parameterize the Chisel-generated mock-ALU Verilog in flow/designs/src/mock-alu/verilog.sh. They are design-specific and not read by any ORFS script, so the correct place to declare them is the design BUILD's user_arguments list, which routes them through orfs_flow(user_arguments=...) to bypass the variables.yaml validator rather than polluting variables.yaml with single-design knobs. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
bazel-orfs's config_mk_parser translates $(DESIGN_HOME) paths in a
config.mk into bazel labels — e.g. an SDC_FILE of
$(DESIGN_HOME)/asap7/aes/constraint.sdc becomes
//flow/designs/asap7/aes:constraint.sdc. The orfs_design() macro
creates per-extension filegroups, but referencing an individual file
across packages requires that file to be in an exports_files() list,
not just inside a public filegroup.
Without this, designs that reuse another design's SDC or shared source
fail at analysis with a Visibility error:
//flow/designs/asap7/aes-mbff -> aes/constraint.sdc
//flow/designs/asap7/gcd-ccs -> gcd/constraint.sdc
//flow/designs/asap7/mock-cpu -> src/mock-array/util.tcl
//flow/designs/asap7/riscv32i-mock-sram
-> fakeram7_256x32/constraints.sdc
//flow/designs/sky130hd/gcd -> src/gcd/gcd.v
Both helpers in flow/designs/design.bzl now call exports_files() over
the same explicit extension list (_EXPORTED_EXTS) the design() macro's
filegroup logic uses — kept tight so a glob("*") doesn't silently
expose LICENSE/.gitignore/etc. as the public API surface.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
bazel-orfs's config_mk_parser turns $(PLATFORM_DIR)/<...> and
$(DESIGN_HOME)/src/cva6/core/include/<...> path references in a
config.mk into per-file labels like //flow:platforms/asap7/lef/foo.lef
or //flow/designs/src/cva6/core/include:bar.svh. Those labels resolve
only if the source package calls exports_files() on the individual
files — being part of a public filegroup is not sufficient.
Without these exports, the bazel synth rule for asap7/cva6 and
asap7/riscv32i fails at analysis with Visibility errors referencing
the platform LEF/LIB/SV files (riscv32i fakeram, cva6 fakeram and
include package files).
Add exports_files glob to:
- flow/BUILD for the full platforms/ tree (asap7/sky130hd/gf180/etc.
all expose similar cross-design references). Exclude BUILD files
so future sub-packages under platforms/ don't conflict.
- flow/designs/src/cva6/core/include/BUILD for the SV package files.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
flow/scripts/flow.sh:20 invokes flow/util/genElapsedTime.py after every OpenROAD-stage run, which appends a one-line "Log Elapsed/s Peak Memory/MB sha1sum result" summary to the stage's log. get_hash() already understands .odb/.rtlil/.v, so it would naturally hash a yosys-stage's result file — but flow/scripts/synth.sh (which runs yosys) didn't call genElapsedTime.py, so the 1_2_yosys.v and 1_1_yosys_canonicalize.rtlil hashes weren't logged. Mirror flow.sh's epilogue inside synth.sh. Informational; sandbox edge-cases (no matching log; result not declared as a bazel action output) must not fail the synth action — route stderr into the log rather than silently dropping it, so a real bug in the helper is still discoverable after the fact. Also rename the column header in genElapsedTime.py from "sha1sum .odb [0:20)" → "sha1sum result [0:20)" since the column has always shown whichever of .odb/.rtlil/.v existed — calling it ".odb" was misleading for the yosys rows the new synth.sh epilogue emits. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Set --lockfile_mode=off in .bazelrc, gitignore the file, and remove the tracked copy. Matches bazel-orfs's own .bazelrc:7. Tracking the lockfile means every bazel-orfs / yosys / openroad bump produces an 800k-line lock diff that buries the real coordinate change under registry-entry churn (the previous yosys 0.63 bump in this branch generated a 1456-line lock delta from a 4-line change in the resolved yosys entry plus 728 lines of registry-fork residue). We pin the versions we care about explicitly in MODULE.bazel via BCR coordinates and git_override(commit=…); the lockfile adds no integrity that those pins don't already provide. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Two small follow-ups to the cross-package exports_files() change: - Include gds and gds.gz in _EXPORTED_EXTS so cross-package ADDITIONAL_GDS references resolve. Brings the exported set in line with the _GROUPS["gds"] entry that already filegroups them. - Factor the duplicated exports_files() glob in design() and files() into a private _export_design_files() helper. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
…helpers Picks up The-OpenROAD-Project/bazel-orfs#732: - config_mk_parser handles shared block.mk fallback for BLOCKS= designs (asap7/aes-block per-block aes_rcon / aes_sbox targets now resolve without a downstream patch). - @bazel-orfs//:yosys-check and @bazel-orfs//:make-yosys-netlist sh_binary helpers ship from bazel-orfs. - "Debugging OpenROAD determinism (bazel vs make)" section in bazel-orfs/TESTING.md. Drops the local bazel/bazel-orfs-patches/ + bazel/yosys-check.sh + bazel/make-yosys-netlist.sh that this PR previously carried, and the mirror determinism section that lived in flow/README.md. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
oharboe
force-pushed
the
bazel-orfs-update
branch
from
2f82049 to
c44b22d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Bazel environment by disabling lockfile tracking, updating the bazel-orfs dependency, and exposing platform and design files as public targets for cross-package visibility. It also introduces new flow variables (LIB_MODEL, MIN_CLK_ROUTING_LAYER, SDC_FILE_EXTRA) and adds result hashing to the synthesis process. Review feedback highlights a potential Bazel error in design.bzl caused by duplicate file exports and suggests optimizing the synthesis script to avoid redundant path evaluations and inefficient file globbing.
Two follow-ups from gemini-code-assist review of PR The-OpenROAD-Project#4229; the other five nits no longer apply (three targeted bazel/yosys-check.sh and bazel/make-yosys-netlist.sh which moved upstream to bazel-orfs in commit 432edcd63; two were already addressed in commit c18e424 when gds/gds.gz were added and the _export_design_files helper was factored out). - flow/designs/design.bzl: make _export_design_files() idempotent via a sentinel filegroup. design() and files() both call it, and a BUILD file may legitimately call files() more than once (e.g. files("verilog") and files("lef") in the same package). A second native.exports_files over the same paths is a duplicate-target error, so the sentinel short-circuits subsequent calls in the same package. The sentinel rule itself is private to the package. - flow/scripts/synth.sh: capture $(realpath "$2") once into log_file rather than re-running it for both the stderr append and the tee target in the genElapsedTime.py epilogue. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
…TRA from variables.yaml
SDC_FILE_EXTRA is set by flow/designs/asap7/mock-cpu/config.mk and
source'd from that design's io.tcl. No ORFS script reads it — it is
a per-design Tcl hook, not part of the flow's shared schema. Earlier
in this branch I declared it in flow/scripts/variables.yaml because
bazel-orfs's config_mk_parser puts SDC_FILE_EXTRA in SOURCE_VARS
(so the path is staged into the sandbox as a label) and orfs_flow
then validates sources.keys() against variables.yaml, which fails
the load phase for an undeclared name. user_arguments= didn't help:
it pops from arguments, not sources.
This commit replaces that workaround with the symmetric knob:
- bazel/0001-orfs_design-add-user_sources.patch carries a
bazel-orfs change adding user_sources=[VAR,...] to orfs_design()
and user_sources={VAR: [label,...]} to orfs_flow(). The named
vars are popped from sources before check_variables() runs, then
merged back in before _orfs_pass so files are still staged.
Shadow guard against known ORFS vars mirrors user_arguments.
- bazel/BUILD exports the patch file, MODULE.bazel applies it to
the pinned bazel-orfs commit (ce6efd9). Drops once upstream
lands an equivalent API.
- flow/designs/design.bzl forwards user_sources= from design() to
orfs_design().
- flow/designs/asap7/mock-cpu/BUILD declares
user_sources = ["SDC_FILE_EXTRA"].
- flow/scripts/variables.yaml, flow/scripts/variables.json, and
docs/user/FlowVariables.md revert the SDC_FILE_EXTRA entry
added earlier in this branch.
Verified: bazelisk query //flow/designs/asap7/mock-cpu:all lists all
stages, and bazelisk cquery --output=build on mock_cpu_synth shows
arguments["SDC_FILE_EXTRA"] = "\$(locations //flow/designs/src/mock-array:util.tcl)"
with util.tcl in data — i.e. the file is still staged.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for user_sources in the ORFS design flow, allowing project-specific source variables to bypass validation while still being staged in the sandbox. It also implements a mechanism to export individual design and platform files for cross-package references, updates the bazel-orfs dependency with a custom patch, and adds new flow variables (LIB_MODEL, MIN_CLK_ROUTING_LAYER). Feedback includes a recommendation to safely define UTILS_DIR in synth.sh to avoid script failures and a suggestion to expand the list of exported file extensions to include .lib.gz and .rtlil.
There was a problem hiding this comment.
What is this for? It seems to relate to private files not in ORFS.
There was a problem hiding this comment.
It will wash out in the next bump, already merged in bazel-orfs. I patch things here to check if they work out to reduce churn in bazel-orfs as the use-case originated here.
Patching to reduce churn and let the use-case settle and then upstreaming is idiomatic for bazel usage.
Any other comments?
2 participants
TL;DR nits and test infrastructure to rule out false positives from yosys non-determinism