rsz: repair_timing -effort policy (tapeout|explore)#10942
Open
oharboe wants to merge 2 commits into
Open
Conversation
An optimization-policy dial in the spirit of the compiler driver -O flags (POLA): the caller states intent, the tool owns the mapping to its internal heuristics. Directives compose left to right, a later directive overrides an earlier one, as in "gcc -O0 -O2". Two are defined: tapeout (default, the exact historical behavior) and explore, which lets setup repair stop when its marginal progress plateaus and does not run hold repair. An explicit -hold to the right of "-effort explore" re-enables hold repair. Graded levels in between are intentionally left undefined. Fixes The-OpenROAD-Project#10900 Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new -effort option to the repair_timing command, providing a coarse optimization policy with directives like tapeout and explore to control setup and hold repair heuristics. Feedback on these changes highlights a few critical issues: the use of a C++20 defaulted comparison operator in EffortPolicy.hh which is incompatible with the project's C++17 standard, a potential glob-matching vulnerability in the Tcl argument parsing, and an uninitialized repaired_hold variable in Resizer.tcl that could trigger Tcl runtime errors when the explore directive is used.
string equal -length instead of string match: an argument carrying glob characters must not confuse the prefix comparison. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
repair_timinghas one built-in policy: work toward tape-out closure. During design space exploration that is the wrong policy — on WNS-dominated designs setup repair grinds for hours toward targets it cannot reach, and hold repair spends hours inserting buffers that carry no exploration information (hold closes at any clock period). Users end up hand-rolling workarounds out of slack margins and skip flags, per stage, per design.This PR adds
repair_timing -effort <directives>: a policy interface analogous to the compiler driver-Oflags (POLA). The caller states intent; the tool owns the mapping to its internal heuristics. Directives compose left to right and a later directive overrides an earlier one, like-O0 -O2. Two directives are defined:tapeout(default) — the exact historical behavior.explore— setup repair may stop when its marginal progress plateaus; hold repair is not run. An explicit-holdto the right of-effort explorere-enables it.The plateau heuristic behind
exploreis a strawman. The ask of this PR is not the heuristic: it is that we agree OpenROAD accepts the policy interface and the responsibility for the mapping behind it. Graded levels between the two (-O1/-O2/-Osanalogues) are intentionally not introduced; that is left for people who understand those use cases.Resolution is implemented in
rsz::EffortPolicywith a gtest unit test;tapeoutreproduces the previous hardcoded values bit-for-bit, so behavior without-effortis unchanged.Measured on the nightly CI of an industrial RISC-V design at three sizes of the same microarchitecture (S/M/L), route stage (global route +
repair_timing). Before = the hand-rolled bounding workarounds that design carried (per-stage slack margins, skip flags); after = those replaced by theexploresemantics of this PR. Detailed routing downstream is unchanged between the two runs (~100 min at S in both), the delta is repair:The two runs are one night apart on actively developed RTL, so the WNS deltas include design churn (S's TNS improved while its WNS regressed); the runtime deltas are the policy.