rsz: Optimize latch time-borrow setup paths#10825
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 support for calculating effective slack at latch endpoints to account for time borrowing setup debt, updating the legacy setup repair flow to target these endpoints correctly. It also adds corresponding integration tests. The review feedback highlights two critical issues: a potential null pointer dereference in latchOutputPins when handling top-level ports, and a significant memory leak and performance bottleneck in latchOutputWorstSlack caused by unnecessary heap allocations and heavyweight path searches, which can be optimized to an O(1) slack lookup.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f53e530931
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
It is confusing to have an sta update in this PR. It would be better to split it off. Is it actually necessary as I think @dsengupta0628 has recently updated sta. |
|
Hi @jhkim-pii , can you please sync to latest OpenROAD master so that I can look at the actual files that you modified other than the bunch of isSequential/hasSequential changes I had to make for getting OpenROAD to work with latest upstream changes in OpenSTA? |
|
PathExpanded::latchPaths() returns a single D to Q hop. When transparent latches sit in series (borrow chains), the returned d_path itself starts at an upstream latch Q that carries its own stored D path - but every call site stops after one level. The But a latch chain is unlikely in ICG paths- so this should be fine for ICG enable latches (depth always 1 unless somebody messes up the design); but latch-based pipeline designs would need chain walk |
Good catch. Do you have any suggestion for the chain walk? (e.g., up to 5 level, unlimited level, ...) |
Treat positive latch time borrowing as hidden setup debt when collecting and repairing setup endpoints. This lets repair_timing optimize paths that report zero slack only because latch transparency is fully consumed.\n\nUse effective endpoint slack during legacy setup repair and accept endpoint improvement when global WNS is unchanged. Add an inferred clock gator regression that verifies repair_timing reduces latch borrow on the enable path.\n\nTesting:\n- cmake --build tools/OpenROAD/build --target openroad -j64\n- ctest --test-dir tools/OpenROAD/build -R '^rsz\.inferred_clock_gator_time_borrow\.tcl$' --output-on-failure Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Compute latch endpoint effective slack from uncovered time borrow instead of subtracting the full borrowed amount. This prevents repair_timing from over-optimizing latch D input paths when the latch output path has enough positive slack to absorb the borrow. Find latch output pins through both direct sequential output pins and output functions that reference internal latch state ports. Add a covered-borrow regression to ensure repair_timing leaves fully covered latch borrow unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Move the RSZ time-borrow regression checks into log-comparison tests so the golden .ok files validate the expected output directly. Add C++ coverage to TestResizer for covered and uncovered latch borrow cases, and wire the shared DEF fixture through both CMake and Bazel. Drop the pass/fail sentinel output from the Tcl tests because the golden log comparison already provides the pass condition. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Replace the iterator-based std::find call with std::ranges::find in the latch output pin discovery path. This resolves the clang-tidy modernize-use-ranges warning reported on the PR diff without changing behavior. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
The inferred_clock_gator_time_borrow and inferred_clock_gator_time_borrow_covered regressions moved from PASSFAIL_TESTS to .ok golden comparison, so a thrown utl::error is no longer required to flag a failure. Replace the utl::error calls with puts, keeping the same message text. On failure the message is now emitted to stdout and caught by the golden .ok diff; the passing golden output is unchanged because these branches only fire when the borrow check fails. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Compute latch output slack directly from the output driver vertex instead of invoking findPathEnds with a temporary SDC exception per latch output pin. This avoids filtered-arrival state churn and heap allocation in the time-borrow effective slack path. Update the covered-borrow regression threshold so it remains covered under the direct vertex-slack calculation. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Initialize the repair target collector before standalone repair_setup_pin uses endpoint effective slack. Keep TNS phase endpoint filtering and progress checks on effective slack so latch time-borrow endpoints collected with hidden setup debt are not skipped by raw STA slack rechecks. Add Tcl coverage for borrowed latch endpoints in repair_setup_pin and TNS repair flows. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Extend setup repair target collection to include the latch D fanin path stored on latch-through timing paths. Rank latch D fanin targets with the downstream path targets so repair_timing can optimize logic that consumed borrowed time instead of only optimizing after the latch Q pin. Update inferred clock gator regression goldens and add C++ coverage for latch D fanin target collection. Test: ctest --test-dir build -R 'TestResizer\.(UncoveredBorrowReducesEffectiveSlack|CoveredBorrowKeepsReportedSlack|LatchThroughPathCollectsLatchDataFaninTargets)' --output-on-failure Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Remove the effective slack endpoint model now that setup repair expands latch-through paths to include the latch D fanin. Use reported STA endpoint slack again for endpoint collection and repair progress checks to avoid creating artificial latch D endpoint violations. Keep the C++ coverage focused on latch-through target collection and rely on the Tcl regressions for end-to-end borrow reduction. Test: cmake --build build -j32 --target openroad TestResizer Test: ctest --test-dir build -R '^TestResizer\.|rsz\.inferred_clock_gator_time_borrow.*\.tcl' --output-on-failure Test: bazelisk test //src/rsz/test:TestResizer //src/rsz/test:inferred_clock_gator_time_borrow-tcl_test //src/rsz/test:inferred_clock_gator_time_borrow_covered-tcl_test //src/rsz/test:inferred_clock_gator_time_borrow_repair_pin-tcl_test //src/rsz/test:inferred_clock_gator_time_borrow_tns-tcl_test Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Refresh the RSZ repair_setup golden logs after the borrowed-latch fanin path selection changes. The behavior now visits additional candidate paths in these small setup repair cases, which changes verbose repair iteration accounting and one multi-pass unbuffer outcome. This keeps the Bazel Tcl regression expectations aligned with the current repair_timing output. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Move the empty violating-pin message from an unconditional info log to the violator_collector debug channel. This keeps speculative or stale endpoint candidate searches from adding noisy INFO output to normal repair_timing regressions. Update the affected repair setup golden logs after the message is hidden by default. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
openroad-ci
force-pushed
the
secure-rsz-time-borrow-opt
branch
from
29c3023 to
5828961
Compare
|
@dsengupta0628 @maliberty Pulled the latest master and cleaned up the commits. |
…row-opt Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
|
|
||
| namespace { | ||
|
|
||
| const sta::Path* latchDataPath(const sta::PathExpanded& expanded) |
There was a problem hiding this comment.
This function is defined twice (in RepairTargetcollector method + here;
You can just have one function in OptimizerTypes.hh/.cc and use in all functions as you have done
const sta::Path* latchDataPath(const sta::PathExpanded& expanded, const sta::StaState* sta);
| const sta::Pin* endpoint_pin = endpoint->pin(); | ||
| sta::Slack endpoint_slack = sta_->slack(endpoint, max_); | ||
| sta::Slack endpoint_slack | ||
| = target_collector_->getCurrentEndpointOriginalSlack(); |
There was a problem hiding this comment.
Replacing the live slack query with getCurrentEndpointOriginalSlack() introduces a stale-slack bug.
getCurrentEndpointOriginalSlack() returns the slack snapshotted in collectViolatingEndpoints() (RepairTargetCollector.cc:810), which runs once at TNS-phase start. setToEndpoint() (RepairTargetCollector.cc:2209) just reads that frozen pair - nothing ever refreshes it. But the TNS loop mutates the design between endpoints: every accepted pass runs updateParasitics() + findRequireds() and commits journal moves. So by the time endpoint k is processed, its snapshot reflects the netlist from before
endpoints 1..k−1 were repaired.
This causes two concrete problems:
- Already-fixed endpoints are re-processed. Endpoints sharing fanin with an earlier-repaired endpoint (exactly the situation in your own test DEF - gated_ff0..3/D all share en_lat and the enable_buf0/1 chain) can be collaterally fixed mid-sweep. The old live check skipped them; the snapshot check re-enters the repair loop for endpoints that already meet margin, costing runtime and area.
- Junk moves get committed on false evidence. prev_endpoint_slack is seeded from the same stale value. On the first changed pass, the acceptance test compares live slack against the stale baseline:
endpoint_slack = sta_->slack(endpoint, max_); // live
const bool endpoint_improved = sta::fuzzyGreater(endpoint_slack, prev_endpoint_slack); // live vs stale
Note also that this change isn't needed for the latch time-borrow mechanism itself: everything the latch fix uses in this loop is live (vertexWorstSlackPath per pass, endpoint list rebuilt at phase start), and latch debt surfaces at downstream endpoints whose live slack is genuinely negative.
A borrow-hidden latch D endpoint (slack 0) isn't rescued by the snapshot either - it was snapshotted with the same sta_->slack() and never entered violating_endpoints_ in the first place. Per the design intent, live slack >= margin is also the correct stopping criterion for latch endpoints (stop when transparent slack >= 0; don't keep driving borrow down once the margin is met).
Suggested fix: revert this line to sta_->slack(endpoint, max_). If there's a specific case that broke with the live query - e.g., an endpoint whose slack flickers above margin mid-sweep while its cone still needs work - could you please share it? That case would be better handled explicitly than by freezing the slack for the whole sweep.
There was a problem hiding this comment.
Thanks for catching this.
The code change was for effective slack solution.
Now effective slack scheme is removed, so this code should be reverted.
3 participants
Summary
Problem
Solution 1 (NOT IMPLEMENTED)
Effective Slack = Slack - Uncovered_borrowuncovered_borrow = max(TimeBorrow - max(Q-output slack, 0), 0), i.e. only the borrow the latch Q-output path cannot absorb.RepairTargetCollector::getEndpointEffectiveSlack()atsrc/rsz/src/RepairTargetCollector.cc.Solution 2 (IMPLEMENTED)
Effective slackis not needed.Impact