Repository navigation
Conversation
Contributor
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the setup repair timing optimization flow by introducing a multi-threaded delay estimator, a sequential timing estimator for flip-flop setup/hold checks, a progress reporter, and a setup repair profiler, while removing several experimental policies. The review feedback highlights several critical robustness issues across the new files, including potential null pointer dereferences, undefined behaviors from unchecked optional accesses or empty vector lookups, and unhandled filesystem exceptions during directory creation.
This was referenced Oct 17, 2026
jhkim-pii
marked this pull request as ready for review
October 17, 2026 05:17
openroad-ci
force-pushed
the
secure-rsz-rebuffer-mt
branch
from
October 17, 2026 05:25
78f8f44 to
6f8bc15
Compare
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
What: the Mt delay estimator models the fanin side loads, the opposite output transition, the latch D-side driver and the flip-flop setup and hold change of a cell swap, and SizeUpMt and VtSwapMt charge them. Why: scoring a swap on the selected path alone let a scored gain turn into a loss after the timing update. How: side models in DelayEstimator and SequentialTimingEstimator, input cap budgets that workers can check without STA, and ArcDelayState renamed to EstimatorContext. The side models are off by default. Setup repair profiler review follow-ups: the profiler constructor takes a RepairSetupProfiler::Mode instead of an int, CRIT_VT_SWAP samples WNS and TNS before its sweep, and the Bazel TestResizerMt target drops the --gtest_filter that skipped the profiler and fixture tests. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: RebufferMt, an Mt buffering move that plans a beam of Rebuffer trees per driver and scores each with the buffer-tree estimator. Why: buffering needs the same scored, comparable candidates as the cell swaps before a batch policy can choose among them. How: makeRebufferTreePlans() and applyRebufferTreePlan() in Rebuffer, estimateBufferTreeWithSidePaths() in DelayEstimator, and current required times after the endpoint scan. No policy runs RebufferMt yet; PARALLEL_EVAL will. LEGACY is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
openroad-ci
force-pushed
the
secure-rsz-rebuffer-mt
branch
from
October 17, 2026 07:06
6f8bc15 to
38357a8
Compare
Member
|
Codex
|
maliberty
approved these changes
Oct 17, 2026
4 tasks done
maliberty
pushed a commit
that referenced
this pull request
Oct 22, 2026
What: the switches this stack added on BufferedNetType (Rebuffer.cc) and on MoveType (SetupParallelEvalPolicy::generatorAllowed()) list every enumerator and have no default label. Why: review of #11798. With a default label the compiler does not warn when a new enumerator is added, so the code that needs a decision for it is not found. How: replace each default label with the missing cases. A function that returns a value keeps a return after the switch, as BufferedNet.cc does. The three "unhandled BufferedNet type" errors RSZ-1009, RSZ-1011 and RSZ-1012 go with their labels. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
maliberty
pushed a commit
that referenced
this pull request
Oct 22, 2026
What: RebufferMtGenerator drops a tree plan that puts a dont-touch load behind a buffer, so no candidate is made from it. Why: review of #11798 (Codex). The tree export skips a dont-touch load and leaves it on the driver net, so the committed tree could differ from the scored one while apply() accepted any positive buffer count. Otherwise the export builds the scored tree: insertBufferBeforeLoads() fails without an error only on a dont-touch net or load, and okToBufferNet() already keeps a dont-touch driver net out. Dropping the tree at generation keeps it out of scoring and batch selection. How: makeTreePlans() adds a plan gate next to the one-buffer and score-cap gates; a tree that keeps the dont-touch load on the driver net stays. RebufferMtCandidate.hh joins the headers the tests see. A unit test marks a load that a planned tree puts behind a buffer as dont-touch and checks that no candidate puts it there. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
maliberty
pushed a commit
that referenced
this pull request
Oct 22, 2026
What: the comment above the buffer-tree calibration factors names the data set they were fitted on and what an off factor costs. Why: review of #11798. It was not clear which data the accuracy report was run on, or whether the factors hold on other PDKs. How: comment only. The factors come from the Nangate45 cases of report_delay_estimator_accuracy_for_rebuffering (the report_delay_estimator_rebuffer_* tests), and no other platform was checked; the estimate only ranks trees, and STA times each batch before it is kept. The comment also gets the report command name right. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
maliberty
pushed a commit
that referenced
this pull request
Oct 22, 2026
What: boundBufferTreeDownstreamStageDelay() says what it bounds and why. Why: review of #11798, which asked whether the floor works around a bad slew estimate. It does: the tree model's wire slew is approximate, and an optimistic slew at the inputs after the tree made the table delta credit those stages with too much delay gain. How: comment only. A stage after the tree keeps 80% of its current delay when its input slew does not improve, falling to 50% as the slew gets sharper. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
maliberty
pushed a commit
that referenced
this pull request
Oct 22, 2026
What: the comment of kBufferTreeHighCapSlewRatio and kBufferTreeHighCapSlewFactor says why a heavily loaded final buffer takes its output slew at a lower effective cap. Why: review of #11798, which asked why this correction is needed. It has the same cause as the effective-cap factors above it: the Liberty lookup uses a lumped cap, while STA's delay calculator drives an RC load with an effective cap below it, and the wire resistance hides the most cap at a final buffer that drives a long load. How: comment only. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
maliberty
pushed a commit
that referenced
this pull request
Oct 22, 2026
What: the comments on the downstream delay floor, the fanin side loss weight, the SizeUpMt score scale and the one-way batch dependency check give their rationale instead of tuning outcomes. Why: review of #11797, #11798 and #11799. The floor comment named a cause that the calibration data does not show, and the other three described tuning runs rather than the reason, unlike the replies posted on those PRs. How: comment only. The floor is a safeguard against crediting the stages after the tree with more gain than the built tree gives; the side loss weight keeps out moves that degrade side paths; a size-up ranks below a VT swap of similar gain because it costs more; a two-way dependency check would defer more candidates. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.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
2 participants
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.
Part 5 of 12 of the PARALLEL_EVAL setup repair stack. It builds on #11797 and also shows the commits of parts 1 to 4; its own change is the last commit.
Summary
Changes
Rebuffer::makeRebufferTreePlans()keeps the best few trees of each timing and area-recovery round (a beam) asRebufferTreePlans, andapplyRebufferTreePlan()inserts one. The serial Rebuffer runs the same rounds with a beam of one.DelayEstimator::estimateBufferTreeWithSidePaths()estimates the target path through a plan and the side paths through the other loads it moves.RebufferMtCandidatescores and commits exactly the tree it was planned with.Stack