gpl: inflate retained timing-repair cells after routability by naveenvenk17 · Pull Request #11899 · The-OpenROAD-Project/OpenROAD

naveenvenk17 · 2026-10-21T14:15:02Z

Retained timing repairs can add cells after ordinary routability refinement has finished. This refreshes RUDY and inflates those surviving new cells without restarting the routability search. Virtual timing iterations and passes where routability refinement is still active do not take this path.

Following @gudeh's review, this now runs by default when timing-driven and routability-driven placement are enabled. I removed the Tcl switch and its C++ option plumbing. Late inflation uses the existing allowance and whole-filler budget; it does not raise target density.

I also merged master, updated the fixtures for the current GPL APIs and fixed the integration test to save the placer-owned routability snapshot before testing restoration. The TileGrid reset fix is separate and has merged as #11917.

After that merge, I moved this PR's test declaration to avoid a BUILD insertion-point conflict. Every target attribute and C++ source/test file is unchanged. All 49 focused cases, the default-behavior Tcl test and BUILD format/lint checks passed again after the move.

Testing on the merged head, on a Linux GCP VM with test-result caching disabled:

All 139 selected CPU GPL, affected resizer and repository checks passed, including the 11 large GPL targets. Four GPU-only targets were excluded.
All 49 focused C++ cases passed in six fresh runs, plus 200 shuffled TileGrid executions.
AddressSanitizer passed 347 case executions. Leak detection was disabled, per the repository configuration.
Four fresh Nangate45 GCD/AES flows compared the old switch enabled against the new default, using the same inputs and seed through legalization, CTS and global routing. All saved result files, instance tables and checked non-runtime measurements matched. Repeated metric keys are compared as complete ordered value sequences, not overwritten.
Both AES runs exercised late inflation after routability completion: a logged 0.854 um^2, funded by a logged 1.382 um^2 of filler removal, with zero remaining compensation and no density change. GCD had no positive inflation and is a compatibility case.
The threaded invariance check still has the same single hardware INFO-line mismatch on both versions. Both produce the same DEF, matching the unchanged upstream golden. This is not counted as a passing test.

The earlier baseline/OFF/ON experiment had mixed tradeoffs: AES TNS and wirelength improved, while WNS, area, buffer count and estimated power worsened. The new matched runs establish equivalence between the old enabled behavior and the new default, not a general QoR improvement. No golden or metric limit was changed. No GPU, detailed-route, signoff-power or speedup claim is made.

Related to #8402.

Carry forward the reviewed changes from The-OpenROAD-Project#11853 under the correct contributor account. The source tree matches 5e30ad2. Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

github-actions · 2026-10-21T14:15:25Z

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 an experimental, opt-in -timing_driven_congestion_inflation option to global_placement. When enabled alongside timing-driven and routability-driven modes, it refreshes RUDY after timing repairs and inflates newly inserted standard cells without restarting the routability search. Feedback on the implementation identifies a potential segmentation fault in GCell::inflationArea() where insts_.front() is accessed without checking if insts_ is empty, and suggests adding a defensive check.

gemini-code-assist · 2026-10-21T14:16:43Z

+int64_t GCell::inflationArea() const
+{
+  if (isFiller()) {
+    return 0;
+  }
+  // Instance geometry carries current master dimensions, placement padding
+  // and pin-density extension. A move callback can make the GCell unpadded;
+  // it must not change this reference or turn padding into congestion area.
+  int64_t physical_area = insts_.front()->getArea();
+  if (insts_.size() > 1) {
+    const GCell physical(insts_);
+    physical_area = static_cast<int64_t>(physical.dx()) * physical.dy();
+  }
+  return std::max<int64_t>(0,
+                           static_cast<int64_t>(dx()) * dy() - physical_area);
+}


In GCell::inflationArea(), calling insts_.front() without verifying if insts_ is empty can lead to undefined behavior or a segmentation fault if the GCell has no associated instances. Adding a defensive check for insts_.empty() ensures robustness.

int64_t GCell::inflationArea() const { if (isFiller()) { return 0; } if (insts_.empty()) { return 0; } // Instance geometry carries current master dimensions, placement padding // and pin-density extension. A move callback can make the GCell unpadded; // it must not change this reference or turn padding into congestion area. int64_t physical_area = insts_.front()->getArea(); if (insts_.size() > 1) { const GCell physical(insts_); physical_area = static_cast<int64_t>(physical.dx()) * physical.dy(); } return std::max<int64_t>(0, static_cast<int64_t>(dx()) * dy() - physical_area); }

isFiller() is defined as insts_.empty(), so the existing early return already guards front().

Regenerated against the current upstream merge and checked for idempotence on the GCP VM. Only the missing Bison registry checksum is added; no dependency versions change. Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

naveenvenk17 · 2026-10-21T15:18:47Z

GCell::isFiller() is exactly insts_.empty(), so the existing return already covers the empty-vector case before front() is reached. I left that check as-is.

gudeh

To approve this PR we need QoR experiments, I should try to run a secure-CI to asses the impact of this.

gudeh · 2026-10-21T16:07:17Z

 value is calculated based on the weight coefficients. The algorithm will stop 
 if the RC is not decreasing for three consecutive iterations.

+The optional `-timing_driven_congestion_inflation` experiment is disabled by


This description is way more complex than necessary.

Shortened this to the option's behavior, its prerequisites and the filler-budget limit.

gudeh · 2026-10-21T16:12:27Z

        = tb_->executeTimingDriven(virtual_td_iter, enable_repair_timing);
    // fixPointers() may have moved the GPins the IO pins point at.
    getTopLevelNB()->refreshIoPins();
+    const bool retained_inflation


We shouldn't do this on TD iterations that happen before routability mode. I am not entirely sure only !is_routability_need_ guarantees that, could be reviewed to make sure.

I checked the state transitions and added regressions for early timing passes, active routability passes, completion and re-entry. The new path requires a retained repair, the opt-in flag, routability mode and !is_routability_need_. That state starts true; only runRoutability() changes it, and any final restore completes before the next timing-repair iteration. The early and active cases leave new cells uninflated.

gudeh · 2026-10-21T16:13:44Z


  const std::vector<GCellHandle>& getGCells() const { return nb_gcells_; }

+  // Pending callback creations, consumed by updateGCellState(). Resolve these


Pending callback creations? Does this ever happen? If so, we may have a bug.

The callbacks are synchronous and have already created the GCells. new_instances_ tracks those creations until the optimizer-state update; it does not represent callbacks waiting to run. I removed the misleading wording and added tests for deduplication, reindexing, deletion and consuming each creation once.

gudeh · 2026-10-21T16:16:17Z

  debugInflatedTiles();
 }

+int64_t RouteBase::inflateNewCells()


Why are inflations being handled here different from the default ones? Can't we have a single function to be used both here (for new instances), and at the original routability inflations?

Both paths now call inflateCell(). Ordinary routability keeps its existing float arithmetic and rounding; late insertions pass an area limit so the result fits the live allowance and available whole fillers. I reran the GCD/AES default-off comparison against upstream plus the separate TileGrid fix in #11917. All 15 saved result files and all checked non-runtime metrics match on both designs.

naveenvenk17 · 2026-10-21T17:10:28Z

Thanks for the review. I'll rename the getter, shorten the README text, and remove the extra comment. I'm also tracing the TD/routability transition and the callback updates, then refactoring the shared inflation logic so the two paths use the same geometry calculation.

I'll post refreshed VM QoR runs after that update. The existing GCD/AES results are mixed, which is why the option is still off by default. A broader secure-CI run would be useful before approval.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

Preserve ordinary float rounding and bounded late filler funding. Document the completed-routability latch and synchronous callback staging, and cover real retained repairs across phase gates and shared geometry rounding boundaries. Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

naveenvenk17 · 2026-10-22T03:02:22Z

Updated this in 6de507e4 with the inline review changes and fresh QoR runs.

I ran baseline, OFF and ON for Nangate45 GCD and AES through legalization, CTS and global routing. The baseline is b4f10aa87d82 plus only the separate TileGrid fix in #11917; using unmodified upstream would hide a real change in the reported congestion metric. OFF matches every checked non-runtime metric and all 15 saved result files for each design.

AES exercised the new path. TNS and wirelength improved, but WNS, area and estimated power got slightly worse. GCD added no inflation. I've put the actual numbers and test scope in the description, and kept this opt-in.

All 47 focused cases passed under ASan, with leak detection disabled. The focused regressions and flow evidence also received independent agent review. Larger secure-CI runs would still be useful before judging whether this is worth keeping.

vvbandeira

approved from devops team side

gudeh · 2026-10-23T00:25:52Z

 global_placement
    [-skip_initial_place]
    [-incremental]
+    [-timing_driven_congestion_inflation]


Lets make this default behavior and remove the TCL parameter.

Done. The switch and C++ option are gone, and the late-inflation path is now the default for timing-driven, routability-driven placement.

I merged master and updated the snapshot/API fixtures too. The merged head passed 139 selected checks, 494 normal native case executions and 347 ASan executions. Fresh GCD/AES runs matched the old switch-enabled behavior through global routing, including the AES late-inflation event. The hardware INFO-line mismatch in mt_invariance01 is unchanged on both versions; the DEFs match the upstream golden.

I also resolved the BUILD conflict with the merged #11917 by moving the test declaration. No target attributes or C++ sources changed. The focused suite and default-behavior Tcl check passed again.

gudeh · 2026-10-23T00:43:26Z

Only reductions in congestion at GPL on private designs.

small improvement on finish wns

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

maliberty

approve for openroad-maintainers

gudeh · 2026-10-24T14:55:28Z

I think we should merge this @maliberty. We seem to have improvements in congestion and and really small (chance level) slack improvement. Other metrics look stable.

I triggered a secure-CI yesterday for large designs, I will check today if all is ok to merge.

maliberty · 2026-10-24T14:57:18Z

@gudeh approve and merge when ready

gpl: add opt-in inflation for post-routability timing insertions

db2bb96

Carry forward the reviewed changes from The-OpenROAD-Project#11853 under the correct contributor account. The source tree matches 5e30ad2. Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

naveenvenk17 requested a review from a team as a code owner October 21, 2026 14:15

naveenvenk17 requested a review from gudeh October 21, 2026 14:15

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

gemini-code-assist Bot reviewed Oct 21, 2026

View reviewed changes

naveenvenk17 requested review from a team as code owners October 21, 2026 14:49

naveenvenk17 requested review from maliberty and sombraSoft October 21, 2026 14:49

naveenvenk17 added 2 commits October 21, 2026 08:09

build: record bison module registry checksum

93d9545

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

build: keep shared lockfile repair out of feature changes

50aaa3b

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

naveenvenk17 mentioned this pull request Oct 21, 2026

build: record the missing bison registry checksum #11902

Closed

gudeh requested changes Oct 21, 2026

View reviewed changes

naveenvenk17 added 3 commits October 21, 2026 17:41

Merge published master into congestion inflation review

e9bfd15

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

Merge shared CI checksum prerequisite into congestion review

d4e9f35

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

naveenvenk17 mentioned this pull request Oct 21, 2026

gpl: reset tile references when rebuilding the routing grid #11917

Merged

vvbandeira approved these changes Oct 22, 2026

View reviewed changes

gudeh requested changes Oct 23, 2026

View reviewed changes

naveenvenk17 added 3 commits October 23, 2026 06:52

gpl: make post-routability timing inflation the default

5a07d35

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

gpl: merge master and adapt timing inflation tests to current APIs

e7c93be

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

gpl: capture placer state in routability integration fixtures

6ed4d8d

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

naveenvenk17 changed the title ~~gpl: add opt-in inflation for post-routability timing insertions~~ gpl: inflate retained timing-repair cells after routability Oct 23, 2026

gpl: move congestion test declaration away from the merged route test

474b618

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

maliberty approved these changes Oct 24, 2026

View reviewed changes

maliberty requested review from gudeh and removed request for sombraSoft October 24, 2026 14:50

maliberty enabled auto-merge October 24, 2026 14:57

gudeh approved these changes Oct 24, 2026

View reviewed changes

maliberty merged commit 5c30883 into The-OpenROAD-Project:master Oct 24, 2026
20 checks passed


		const std::vector<GCellHandle>& getGCells() const { return nb_gcells_; }

		// Pending callback creations, consumed by updateGCellState(). Resolve these

Conversation

naveenvenk17 commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 21, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveenvenk17 commented Oct 21, 2026

Uh oh!

gudeh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveenvenk17 commented Oct 21, 2026

Uh oh!

naveenvenk17 commented Oct 22, 2026

Uh oh!

vvbandeira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveenvenk17 Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gudeh commented Oct 23, 2026

Uh oh!

maliberty left a comment

Choose a reason for hiding this comment

Uh oh!

gudeh commented Oct 24, 2026

Uh oh!

maliberty commented Oct 24, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

naveenvenk17 commented Oct 21, 2026 •

edited

Loading

naveenvenk17 Oct 23, 2026 •

edited

Loading