test: check flow metrics under bazel - #11202
Conversation
The bazel flow tests ran the RTL-to-GDS flow but checked only the exit code: BIG_TESTS get check_log=False, and PASSFAIL_TESTS is just commands_without_load, so check_passfail was False too. The <design>.metrics_limits files were read only by flow_metrics.tcl, which only ./regression sources, so a green bazel run said nothing about QoR. Add a check_metrics attribute to regression_rule_test. When set, regression_test.sh passes -metrics so openroad writes the result json, then compares it against <design>.metrics_limits and fails the test on a violation. Enabled for the 13 BIG_TESTS that have a limits file, derived from a glob so a new design opts in by adding one. The comparison reuses the metric table and margins from flow_metrics.tcl via a new compare_metrics_files, so bazel and ./regression apply the same limits and report the same message. It reads the json with a new read_flat_json rather than tcllib's json package, which the bazel-built openroad does not carry; the metrics files are machine written flat objects of scalars, and nested json is rejected rather than guessed at. read_flat_json returns dicts identical to json::json2dict for all 31 metrics and limits files in the tree. This surfaces four pre-existing QoR violations under bazel: aes_asap7 (also fails ./regression, so its limit is genuinely stale), plus aes_nangate45, aes_sky130hs and jpeg_sky130hs, which pass ./regression and differ only because the bazel binary's QoR differs from cmake's. The limits are unchanged here; these tests stay tagged manual, so default CI is unaffected. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Turning on the bazel metrics check surfaced four designs whose QoR was outside their limits: aes_asap7, aes_nangate45, aes_sky130hs and jpeg_sky130hs. Their limits were last derived from cmake ./regression runs, but CI runs the flow tests under bazel, whose -O3 -flto binary produces different QoR. Regenerate the metrics and limits for those four from bazel runs, using the metrics json the check now writes as an undeclared test output. All 22 gated metrics are still present in each limits file, so the gate is not weakened; the limits move both ways, tightening where bazel is better (aes_nangate45 max_capacitance_slack -33% -> -20%) and loosening where it is worse (aes_asap7 max_slew_slack -11% -> -38%). Deriving limits from bazel means ./regression can now fail these designs where cmake QoR is worse than bazel's: aes_nangate45 on max_capacitance_slack, aes_sky130hs on clock_skew and ANT errors, and jpeg_sky130hs on ANT errors. One set of limits cannot satisfy both binaries for these designs, and bazel is what CI gates on. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
gcd_sky130hd_fast_slow was registered only in test/BUILD, not in regression_tests.tcl or CMakeLists.txt. CI runs it as its own bazel flow stage, but with check_log and check_passfail both false and no metrics_limits file it asserted nothing beyond openroad exiting 0, leaving it the one flow design with no output checked at all. It had no limits file because the limits tooling could not produce one: save_metric_limits indexes test_langs($test), so save_flow_metrics_limits reported 'test not found'. Add it to record_flow_tests, which lets the tooling generate its limits and makes ./regression able to run it like the other flow designs, then save its metrics and limits from a bazel run. It agrees between the two binaries, passing under both bazel and ./regression. Every BIG_TEST now has a limits file, so drop FLOW_METRIC_TESTS and check metrics for all of them. Selecting on the presence of a limits file would silently leave a new design unchecked, which is how this one went unnoticed; without a limits file the check now fails with 'missing metrics limits file' instead. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
There was a problem hiding this comment.
Code Review
This pull request integrates flow metrics validation into the Bazel-based regression test suite. It introduces a custom, lightweight JSON parser in Tcl (test/flow_metrics.tcl) to avoid external dependencies like tcllib, adds a standalone script (test/check_metrics.tcl) to compare metrics against limits, and updates the Bazel rules and shell scripts to support the new check_metrics option. Additionally, a new test case (gcd_sky130hd_fast_slow) is added, and several existing test metrics are updated. The reviewer suggested a portability improvement in test/regression_test.sh to use "$0" instead of "${BASH_SOURCE[0]}" to ensure compatibility with non-Bash shells.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…check Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
2930799 to
e35fe41
Compare
cc40ffd
into
The-OpenROAD-Project:master
Summary
The bazel flow tests ran the full RTL-to-GDS flow and then checked only the
exit code.
BIG_TESTSgetcheck_log = False, andPASSFAIL_TESTSis justcommands_without_load, socheck_passfailwasFalseas well. The<design>.metrics_limitsfiles were read only bytest/flow_metrics.tcl, whichonly
test/regression.tclsources, so QoR was checked by./regressionand bynothing else. A green
bazel test //test:flow_testssaid nothing about QoR.This adds a
check_metricsattribute toregression_rule_test. When set,regression_test.shpasses-metricsso openroad writes the result json, thencompares it against
<design>.metrics_limitsand fails the test on a violation.It is enabled for every
BIG_TEST, since those produce no golden log and theirQoR is all there is to check. A new design must save a
<design>.metrics_limitsfile; without one the check fails with "missing metrics limits file" rather than
running the flow and asserting nothing.
The comparison reuses the existing metric table and margins through a new
compare_metrics_files, so bazel and./regressionapply identical limits andprint identical messages.
Reading the json without tcllib
compare_metrics_filescannot use tcllib'sjsonpackage: the bazel-builtopenroad links core
tcl_langand has no tcllib, so the first attempt failedwith
can't find package json. Vendoring tcllib for this is far more than thecheck is worth, and depending on the host's tcllib would make the test
non-hermetic.
So it reads the json with a new
read_flat_json. The metrics and limits filesare machine written by
utl::metricandsave_metric_limitsand are always oneflat object of string or number values, which is also all
compare_metrics_filescan consume. Nested objects and arrays are rejected rather than guessed at, so a
format change fails loudly instead of comparing the wrong numbers.
read_flat_jsonreturns dicts identical tojson::json2dictfor all 31 metricsand limits files in the tree (keys and values, zero mismatches). It rejects
nested objects, arrays, a non-object top level, unterminated objects and strings,
unquoted keys, and a missing colon; it handles string escapes, exponent and
negative numbers,
true/false/null, and empty objects. The other fivejson::json2dictsites inflow_metrics.tclare report and save paths that runonly under
./regression, where tcllib is present, so they are left alone.Impact
Two commits: the first adds the mechanism, the second regenerates the limits it
found to be out of date.
Turning the check on surfaced four designs whose QoR was outside their limits.
Their limits had last been derived from cmake
./regressionruns, but CI runsthe flow tests under bazel (jenkins-ci
vars/pipelineOR.groovygives each designits own
Bazel Flow Test <design>stage, naming//test:<design>-tcl_testexplicitly to bypass the
manualtag), whose-O3 -fltobinary producesdifferent QoR. So these had been drifting in CI behind a green tick.
aes_asap7DRT::max_slew_slack -32% < -11%aes_nangate45RSZ::hold_buffer_count 78 > 37aes_sky130hsworst_slack_max,max_slew_slack,max_capacitance_slackjpeg_sky130hshold_buffer_count,clock_skew,max_capacitance_slackThe limits are regenerated from the metrics json the check now writes as an
undeclared test output, so bazel runs are self-sufficient for this. All 22 gated
metrics remain in every limits file, so the gate is not weakened. The limits move
both ways: tighter where bazel is better (
aes_nangate45DRT::max_capacitance_slack-33% to -20%), looser where it is worse(
aes_asap7DRT::max_slew_slack-11% to -38%).gcd_sky130hd_fast_slowThis design was registered only in
test/BUILD, not inregression_tests.tclorCMakeLists.txt. CI gave it its own bazel flow stage, but withcheck_logandcheck_passfailboth false and no limits file it asserted nothing beyond openroadexiting 0 — the one flow design with no output checked at all.
It had no limits file because the tooling could not produce one:
save_metric_limitsindexestest_langs($test), sosave_flow_metrics_limitsanswered "test not found". Adding it to
record_flow_testslets the toolinggenerate its limits and lets
./regressionrun it like the other designs. Itsmetrics and limits come from a bazel run, and it agrees between the two binaries,
passing under both bazel and
./regression. All 14 flow designs are now checked../regressionconsequenceDeriving limits from bazel means
./regressionnow fails these designs wherevercmake QoR is worse than bazel's:
aes_nangate45—RSZ::max_capacitance_slack -14% < -11%,DRT::max_capacitance_slack -23% < -20%aes_sky130hs—DRT::clock_skew 0.29 > 0.13,DRT::ANT::errors 1 > 0jpeg_sky130hs—DRT::ANT::errors 1 > 0(
aes_asap7now passes both.) One set of limits cannot satisfy both binaries forthese designs, and bazel is what CI gates on, so bazel wins. The alternative, if
keeping
./regressiongreen on these matters, is to take the worse of the twoengines per metric, which weakens the CI gate to the cmake number.
Verification
bazel test //test:flow_tests— 10 of 14 passed before regeneration; the 4 above failed on metricsonly, each with
Exitcode: 0, so the flows themselves succeed. After regeneration all 4 pass under bazel.gcd_nangate45'sDPL::design_arealimitto
1.0makes bazel reportMetrics do not satisfy limits: DPL::design_area 615 >= 1and fail, and./regressionreport*FAIL* DPL::design_area 615 >= 1. Restored after.ctest— 8338/8338 pass, confirming the sharedregression_test.shchangedoes not disturb non-flow tests.
bazel test //test:upf_aes-tcl_test //test:write_db-tcl_test //src/psm/test:report_writers_require_solution-tcl_test— pass.buildifier,tclfmtandtclintclean.