rsz: add default max fanout backstop - #11197
Conversation
sta::checkFanout reports no fanout limit when neither the liberty libraries nor the sdc constrain fanout, and rsz then skipped fanout repair entirely. High fanout nets, for example a synchronous reset network, were left on a single driver for repair_design to attack through max slew and max cap alone, which splits the wire instead of building a fanout tree. Add Resizer::checkFanout, which passes an existing liberty or sdc limit through untouched and otherwise falls back on a default of 50 load pins, so those nets still get a buffer tree. It counts load pins itself rather than liberty fanout load, so the backstop applies to libraries with no fanout load data as well, and it skips the pins OpenSTA leaves out of the fanout check: non drivers, constants, disabled pins and ideal clocks. repairNet, the gain buffering round and the rebuffer tree limit all go through it. The unbuffer move tested its own limit with `limit > 0.0`, but sta::checkFanout reports an infinite limit rather than a zero one when nothing constrains the pin, so every removal passed the guard and the kBufferRemovalMaxFanout default of 10 was unreachable. Test the limit against INF so the default applies again. Signed-off-by: Minju Kim <mkim@precisioninno.com>
The flow tests in test/ are not part of ctest, so the backstop's effect on
them was missed. Six of the thirteen fail their metric limits: the backstop
buffers high fanout nets that used to stay on one driver, which trades setup
slack and buffer count for slew and capacitance slack.
Failing gates, old value -> new value (limit):
aes_sky130hd RSZ::repair_design_buffer_count 580 -> 932 (696)
DRT::worst_slack_max -1.357 -> -1.886 (-1.731)
aes_sky130hs RSZ::repair_design_buffer_count 722 -> 980 (866)
DRT::max_slew_slack 5.8% -> -4.5% (0%)
DRT::max_capacitance_slack 6.5% -> -6.4% (0%)
aes_asap7 RSZ::max_slew_slack 0.2% -> -16.9% (0%)
DRT::worst_slack_max -108.3 -> -242.7 (-148.3)
DRT::max_slew_slack -9.5% -> -79.2% (-11.4%)
jpeg_sky130hd RSZ::repair_design_buffer_count 295 -> 622 (354)
DRT::max_capacitance_slack -10.0% -> -12.4% (-12.0%)
jpeg_sky130hs RSZ::repair_design_buffer_count 538 -> 770 (645)
DRT::max_capacitance_slack 11.0% -> -0.9% (0%)
ibex_sky130hs DRT::ANT::errors 0 -> 1 (0)
The other direction on the same designs: DRT antenna errors drop 1 -> 0 on
aes_sky130hd, aes_sky130hs, jpeg_sky130hd and jpeg_sky130hs, DRV slack
improves on aes_sky130hd (max slew -26.6% -> -11.4%, max cap -28.8% ->
-13.0%), jpeg_sky130hs (max slew -47.6% -> 0.1%) and ibex_sky130hs (max slew
-51.2% -> 1.2%, max cap -51.4% -> 0.0%), and design area drops on the aes and
ibex runs. gcd on all four platforms, aes_nangate45, ibex_sky130hd and
tinyRocket_nangate45 still pass untouched.
Two of these are regressions the limits now accept rather than golden churn:
aes_asap7 loses 134 ps of routed setup slack and ibex_sky130hs gains one
antenna violation.
Regenerated with save_flow_metrics and save_flow_metrics_limits; all thirteen
flow tests pass afterwards.
Signed-off-by: Minju Kim <mkim@precisioninno.com>
![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 a default fanout limit backstop of 50 load pins in the resizer when no liberty or SDC constraints are present. It implements Resizer::checkFanout to apply this limit, updates RepairDesign and Rebuffer to utilize the new check, and adds a test case (repair_fanout9) to verify the behavior. Metric updates across several test cases reflect the new buffering and timing results. There are no review comments, so no feedback is provided.
|
@oharboe fyi |
|
My main concern is that we're missing tests that create a gradient towards better policies here. I asked agy and it says that repair and buffer insertion is part of the rsz package and that resizing/sizing is a bit of a historical name and that rsz really is a post placement optimization engine. Matt reverted(or will revert) a naive policy with an upper bound of buffering that agy came up with when I asked it for fix, and I thought sounded plausible and merited some consideration, after it observed that a design I worked on added more buffering because wires got longer because it was harder to find places for buffers, which added more wiring, which added more buffering... This ran for so long that I considered it "stuck". The test case I was working on had lots of macros and a large die and paths that crossed the die, much like MegaBOOM on asap7. asap7 has MUCH higher wire resistance(or should at 7nm) than sky130hd, so a test case for asap7 is qualitatively different w.r.t. algorithms than sky130hd test-cases. |
|
Regarding testing, I don't think ORFS, nor OpenROAD contains an asap7 design with a large die, macros where wires have to route around and very little space left for buffers. I think synthetic test case of this kind could have some value if it could be fast. I imagine that any sufficiently well covered rsz policy in ORFS will make some things worse and some better, rarely does a new algorithm dominate all other implementations on the pareto front, so my prior is that ORFS lacks designs that test this new policy. |
|
I have a hard time seeing how "golden"(rules-base.json, .ok/.okdwf or "testing that it is working as implemented") could help test a policy that handles large global placement problems where we want a gradient towards improving the pareto front of the global placement algorithm. I see you ran into aes_upf. I tried to convert aes_upf to testing intent #11152, but it had a run-in with Gemini nits, shot my self in the foot, so I closed it until I would have some time to do it properly. C++ unit-tests is a mechanism, they too can "test what is implemented", rather than test intent. So, I don't think C++ unit-tests, the mechanism, will magically test that we are improving the global placement algorithm. A .tcl test could equally well test improvements in global placement aglorithms. I think that great tests document the intent, verify against a specification and creates a gradient to improve the product, move the pareto front, when comparing against the merge-base. |
3 participants
Summary
sta::checkFanoutreports no limit when neither the liberty libraries nor the SDC constrain fanout, so rsz skipped fanout repair entirely and high fanout nets stayed on one driver.Resizer::checkFanoutpasses an existing liberty or SDC limit through and otherwise falls back on a default of 50 load pins. It counts load pins itself, so no OpenSTA change is needed and libraries with no fanout load data are covered too. It skips the pins OpenSTA leaves out of the check: non drivers, constants, disabled pins, ideal clocks.Also fixes the unbuffer move, which tested
limit > 0.0whilesta::checkFanoutreports INF for an unconstrained pin, leaving itskBufferRemovalMaxFanoutdefault of 10 unreachable.Type of change
Impact
Designs with no max fanout limit now get fanout trees. On
upf_aes: 3 fanout violations repaired, 44 buffers against 4, gpl timing-driven repair_design area recovery -24.70% -> -22.92%. Onrepair_slew8(250 load reset net): area +2.8% against +2.2%, one -0.04 max slew violation left.Verification
psm.report_writers_require_solution, is a test isolation issue unrelated to this change (its own previous run leaves the file it asserts absent); it passes with a clean results dir.repair_fanout9covers a library with no fanout load data.Related Issues
N/A