rsz: Add the per-move analysis and the rebuffer accuracy report by jhkim-pii · Pull Request #11804 · The-OpenROAD-Project/OpenROAD

jhkim-pii · 2026-10-17T04:47:00Z

Part 11 of 12 of the PARALLEL_EVAL setup repair stack. It builds on #11803 and also shows the commits of parts 1 to 10; its own change is the last commit.

Summary

Add two debug tools: profile mode 3, which records what every move did to timing, and an accuracy report for the buffer-tree estimate.

Changes

-profile_mode 3: before each move, RepairSetupAnalysis snapshots the target path, its pins, arcs and connections; after the timing update it samples WNS, TNS, the target slack and the estimator window delay. RepairSetupAnalysisReport writes CSV files under analysis/ and a Markdown report for each flagged move (bad_moves.md).
Mode 3 observes moves without steering them: the core updates parasitics and timing around each move, and the analysis only reads STA. It prints RSZ-2031 when it starts.
report_delay_estimator_accuracy_for_rebuffering -inst inst [-driver_pin pin] [-delay_levels n] plans and scores the trees of a driver as RebufferMt does, applies the best one inside an ECO journal, prints the estimate next to STA with the side paths it charged, and undoes the tree.
The profiler records one event per estimate and per generator and reports every output setup failure with a message ID.

flowchart LR
  subgraph M["profile mode 3, per move"]
    A["snapshot"] --> B["apply the move"] --> C["RC and STA update"] --> D["sample and compare"] --> E["analysis/*.csv,<br/>bad_moves.md"]
  end
  subgraph R["rebuffer accuracy report"]
    P["plan and score the trees<br/>as RebufferMt does"] --> Q["estimate the best one"] --> X["apply in an ECO journal"] --> Y["measure with STA"] --> Z["print both, undo"]
  end

Behavior

With -profile_mode below 3, logs and results stay the same. Mode 3 is for debugging: it costs about twice the run time and writes large files.

Stack

test: Add LEGACY repair setup path regressions #11794 test: Add LEGACY repair setup path regressions
rsz: Remove MT1, MEASURED_VT_SWAP, LEGACY_MT and the RSZ_* knobs #11795 rsz: Remove MT1, MEASURED_VT_SWAP, LEGACY_MT and the RSZ_* knobs
rsz: Add the setup repair profiler #11796 rsz: Add the setup repair profiler
rsz: Model side paths in the Mt delay estimator #11797 rsz: Model side paths in the Mt delay estimator
rsz: Add the RebufferMt buffer-tree move #11798 rsz: Add the RebufferMt buffer-tree move
rsz: Add the experimental PARALLEL_EVAL setup repair policy #11799 rsz: Add the experimental PARALLEL_EVAL setup repair policy
rsz: Add SplitLoadMt to PARALLEL_EVAL #11800 rsz: Add SplitLoadMt to PARALLEL_EVAL
rsz: Add SwapPinsMt to the PARALLEL_EVAL WNS tail #11801 rsz: Add SwapPinsMt to the PARALLEL_EVAL WNS tail
rsz: Add UnbufferMt to the PARALLEL_EVAL WNS tail #11802 rsz: Add UnbufferMt to the PARALLEL_EVAL WNS tail
rsz: Add SizeDownFanoutMt to the PARALLEL_EVAL WNS tail #11803 rsz: Add SizeDownFanoutMt to the PARALLEL_EVAL WNS tail
rsz: Add the per-move analysis and the rebuffer accuracy report #11804 rsz: Add the per-move analysis and the rebuffer accuracy report (this PR)
doc: Update the rsz setup repair architecture #11805 doc: Update the rsz setup repair architecture

github-actions · 2026-10-17T04:47:15Z

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

Code Review

This pull request introduces the experimental PARALLEL_EVAL setup repair policy, which optimizes setup timing in parallel batches. It adds multi-threaded move generators (RebufferMt, SizeDownFanoutMt, SplitLoadMt, SwapPinsMt, UnbufferMt, VtSwapMt, and SizeUpMt), integrates a sequential timing estimator for flip-flop setup/hold checks, and introduces profiling and diagnostic tools like report_delay_estimator_accuracy_for_rebuffering. The review feedback highlights several critical issues and optimization opportunities, including a bug in SwapPinsMtCandidate::estimate() where non-improving swaps are not marked illegal, potential undefined behavior in RebufferPlannerPool::acquire(), and performance enhancements such as replacing std::hypot with std::sqrt in hot paths, using std::bit_cast, and avoiding unnecessary copies of RebufferTreePlan.

jhkim-pii · 2026-10-17T05:44:06Z

@codex review

chatgpt-codex-connector · 2026-10-17T05:44:28Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-17T05:51:46.862666Z	`ae428a9`	Manual request

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector · 2026-10-17T05:51:46Z

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Failed to sample tokens

ℹ️ 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".

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>

What: PARALLEL_EVAL, an experimental setup repair policy that commits a batch of VtSwapMt, SizeUpMt and RebufferMt moves per timing update. Why: one timing update per batch instead of per move cuts the repair time. How: worker threads scan endpoints and generate and score candidates from prepared data; BatchSelector, BottleneckAnalysis and ParallelEvalState pick, rank and stop; each batch is kept, salvaged or rolled back by its WNS and TNS (the group's under -path_group). OptimizationPolicy::finish() ends each phase, and the Optimizer prints the final report after the last phase. LEGACY results do not change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

What: SplitLoadMt, the Mt form of SplitLoad, joins the PARALLEL_EVAL default sequence. Why: splitting off the less critical loads is a strong setup move that PARALLEL_EVAL lacked. How: estimateLoadChange() scores the lighter target load and the moved loads are charged as side loads; the split itself is chosen as in the serial move. LEGACY is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

What: SwapPinsMt, the Mt form of the pin swap, joins the PARALLEL_EVAL WNS tail. Why: a pin swap is a cheap gain on the worst endpoints once the batches stall. How: Resizer::swapPinDelays() is shared with the serial move; the Mt generator measures the arc delays on the main thread and picks the fastest input on a worker. LEGACY is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

What: UnbufferMt, the Mt form of buffer removal, joins the PARALLEL_EVAL WNS tail. Why: removing a buffer on the worst path is a gain the other Mt moves cannot make. How: the main thread checks the buffer and estimates the saved delay; the tail removes buffers only in single-move batches, since a removal cannot be reverted in place. LEGACY is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

What: SizeDownFanoutMt, the Mt form of SizeDownFanout, joins the PARALLEL_EVAL WNS tail, and every Mt generator follows the same steps: isApplicable() and prepareInputs() on the main thread, generate() and estimate() on the workers. Why: the last serial move with an Mt form, and one rule for which code may read live STA. How: SizeDownFanoutMt measures the step-down cell per off-path load; the virtual MoveGenerator::prepareInputs(Target&) replaces the per-move prepare branches of the policy. PARALLEL_EVAL also stops at the walked -max_utilization area. LEGACY is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

What: profile mode 3 records what every move did to timing, and report_delay_estimator_accuracy_for_rebuffering compares the buffer-tree estimate of a driver with STA. Why: to find the moves whose estimate misled the repair and to measure the buffer-tree estimator. How: RepairSetupAnalysis snapshots each move and samples timing after it; the report plans the trees as RebufferMt does, applies the best one in an ECO journal and undoes it. Lower profile modes do not change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

maliberty · 2026-10-18T04:46:41Z

Claude

== Priority 1 (correctness) ==

src/rsz/src/RepairSetupAnalysis.cc:217-224 bug: windowDelay() looks up the vertex of
endpoint_pin_name_ but matches it against path_points_, which were captured from
selected_path_end_. When the two differ (driver-prefix targets, where captureWindow
still runs), matchesPath always fails, so window_delay_s, actual_window_gain_s and
estimate_error_s are empty with no warning. Key on selected_path_end_?
src/rsz/src/Resizer.tcl:1182-1185 bug: -driver_pin is not checked to belong to -inst or
to be an output. Another instance's pin gets rebuffered while the header says
"instance: "; an input pin of -inst gets treated as the driver.
src/rsz/src/RepairSetupAnalysisTarget.cc:63-65 question: when source.endpoint_pin is
null, refresh() returns the policy's original target. Its endpoint_path/driver_path
date from before the updateTimingForObserver() call (SetupLegacyBase.cc:698-703) and
are then dereferenced in RepairSetupAnalysis. Can the arrival update reallocate the
vertex paths? The same applies to the candidate.target() fallback at
RepairSetupProfiler.cc:631 and to the one-argument moveSelected in PARALLEL_EVAL
(SetupParallelEvalPolicy.cc:567).
src/rsz/src/DelayEstimatorReporter.cc:290-299 question: ~EcoJournalScope catches
everything and returns quietly. If undoEco or the timing update fails, the test
buffers stay in the design with no message. Warn at least?
question: the PR says mode 3 "observes moves without steering them", yet it updates
parasitics and required times before each candidate and around each apply, and RSZ-2031
warns that results can change. Have mode 2 and mode 3 been compared for identical output?

== Priority 2 (QoR) ==

DelayEstimatorReporter.cc:574-592 question: planRebufferReport picks the best tree with its
own max-score logic rather than the policy's selector, so it can drift from what
PARALLEL_EVAL actually commits. When no candidate is legal it reports the first tree
without saying so.

maliberty · 2026-10-18T04:47:03Z

Codex

Bug — DelayEstimatorReporter.cc (src/rsz/src/DelayEstimatorReporter.cc:1337): When -driver_pin is supplied, the report never checks that the pin belongs to -inst. I reproduced this with -inst r2 -driver_pin r1/Q: the command succeeds and labels the report as instance r2 while analyzing r1/Q. The instance is also passed into estimator context construction, so the resulting comparison can be misleading. Please reject a mismatched pin and add a command test for it.

maliberty · 2026-10-18T14:19:39Z

+// Captures timing and topology before and after one ECO move for analysis.
+// Construct it before apply() on the main thread; the caller refreshes RC and
+// STA before the second sample.
+class RepairSetupAnalysis


This name is confusing as it isn't really a full analysis but more of a capture of state. Perhaps RepairSetupMoveSampler ?

maliberty · 2026-10-18T14:22:49Z

Similarly RepairSetupMoveSnapshot.hh

maliberty · 2026-10-18T14:34:14Z

+    if { [lsearch -exact {0 1 2 3} $profile_mode] == -1 } {
+      utl::error RSZ 3243 "-profile_mode must be 0, 1, 2 or 3."


Do such validation in c++ so it covers all APIs and not just TCL

jhkim-pii self-assigned this Oct 17, 2026

github-actions Bot added the size/XL label Oct 17, 2026

gemini-code-assist Bot reviewed Oct 17, 2026

View reviewed changes

jhkim-pii marked this pull request as ready for review October 17, 2026 05:17

jhkim-pii requested review from a team as code owners October 17, 2026 05:17

jhkim-pii requested a review from precisionmoon October 17, 2026 05:17

openroad-ci force-pushed the secure-rsz-move-analysis branch from ae428a9 to c9e0c86 Compare October 17, 2026 05:45

jhkim-pii added 7 commits October 16, 2026 23:51

openroad-ci force-pushed the secure-rsz-move-analysis branch from c9e0c86 to f2f551c Compare October 17, 2026 07:26

maliberty approved these changes Oct 18, 2026

View reviewed changes

maliberty merged commit f14e554 into The-OpenROAD-Project:master Oct 18, 2026
20 checks passed

maliberty deleted the secure-rsz-move-analysis branch October 18, 2026 14:35

		if { [lsearch -exact {0 1 2 3} $profile_mode] == -1 } {
		utl::error RSZ 3243 "-profile_mode must be 0, 1, 2 or 3."

Conversation

jhkim-pii commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Behavior

Stack

Uh oh!

github-actions Bot commented Oct 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhkim-pii commented Oct 17, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot commented Oct 17, 2026

Uh oh!

maliberty commented Oct 18, 2026

Uh oh!

maliberty commented Oct 18, 2026

Uh oh!

maliberty Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jhkim-pii commented Oct 17, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Oct 17, 2026 •

edited

Loading