Gpl incremental

[LucasYuki]

Summary

Reworks GPL's incremental placement (Replace::doIncrementalPlace()), which places newly-added/unplaced instances with everything else locked:

• Phase 1 (the locked placement pass) now redistributes the filler GCells into free bin capacity before it runs. Its fixed iteration cap is also raised from 300 to 600, and the target overflow was changed from 0.2 to 0.1.
• Phase 2 (the finishing pass, run after unlocking) is reworked from a single unconditional pass into an incremental density-penalty guard: it only runs when phase 1 actually diverged or missed the real target overflow (checked against NesterovPlace::getAverageOverflow(), not just a fixed proxy threshold), recovers from phase-1 divergence instead of letting it abort the whole incremental call, and escalates (doubles) the density penalty in place whenever overflow regresses past its best-seen value, instead of applying one fixed penalty and hoping it converges.

Type of Change

• New feature
• Refactoring

Impact

• Filler cells are repositioned into free capacity ahead of phase 1, rather than randomly placed.
• Phase 1 target overflow was changed to 0.1
• Phase 2 is skipped entirely when phase 1 has already reached its target overflow, instead of always running a second full pass.
• When phase 2 runs, it tracks the best overflow seen so far and doubles the density penalty in place every time overflow regresses past it, instead of applying one fixed penalty for the whole pass.
• No public API changes to PlaceOptions; behavior changes are internal to doIncrementalPlace().

Verification

• I have verified that the local build succeeds (./etc/Build.sh).
• I have run the relevant tests and they pass.
• My code follows the repository's formatting guidelines.
• I have included tests to prevent regressions.
• I have signed my commits (DCO).

Related Issues

[gemini-code-assist]

Code Review

This pull request introduces an incremental density-penalty guard and a filler cell redistribution mechanism to improve convergence and stability during incremental placement. Key changes include the addition of redistributeFillerCells in NesterovBase to distribute filler cells into free bin capacity, and the implementation of guardIncrementalDensityPenalty in NesterovPlace to escalate the density penalty factor when overflow regresses. Feedback on these changes highlights several critical improvement opportunities: adding defensive null checks for np_ in replace.cpp to prevent potential segmentation faults, capping the density penalty growth factor to avoid numerical overflow or NaN propagation, and validating or resetting coordinates if Phase 1 diverges before proceeding to Phase 2.

[LucasYuki]

I ran the flow with these modifications (using The-OpenROAD-Project/OpenROAD-flow-scripts#4594).
4 designs triggered the module swap, and the max overflow during incremental placement was much lower with these changes, resulting in far fewer changes to the original global placement.

Platform / Design	Master max overflow	This PR max overflow
asap7 / cva6	0.7897	0.1078
ihp-sg13g2 / riscv32i	0.9007	0.1199
nangate45 / swerv	0.8272	0.1014
sky130hs / jpeg	0.7772	0.1585

Of these designs, only the asap7 / cva6 reached the second phase, and in this phase (with all instances unlocked) only 6 iterations were necessary to reach the target overflow. This is the design that was having a lot of degradation due to the incremental mode, as described in issue 11358.

The final timing metrics of these designs were:

Setup WS (ns)

Design	Master	This PR
asap7 / cva6	-43.22	-23.64 (+45.3%)
ihp-sg13g2 / riscv32i	-2.46	-1.90 (+22.9%)
nangate45 / swerv	-0.65	-0.62 (+3.6%)
sky130hs / jpeg	-0.27	-0.20 (+23.8%)

Setup TNS (ns)

Design	Master	This PR
asap7 / cva6	-1249.2	-848.8 (+32.1%)
ihp-sg13g2 / riscv32i	-2108.3	-1730.2 (+17.9%)
nangate45 / swerv	-447.9	-239.8 (+46.5%)
sky130hs / jpeg	-38.5	-17.6 (+54.3%)

Hold WS (ns)

Design	Master	This PR
asap7 / cva6	24.961	24.233 (-2.9%)
ihp-sg13g2 / riscv32i	0.233	0.233 (+0.0%)
nangate45 / swerv	0.016	0.011 (-32.2%)
sky130hs / jpeg	0.193	0.181 (-6.2%)

Fmax (MHz)

Design	Master	This PR
asap7 / cva6	1049.08	1071.07 (+2.1%)
ihp-sg13g2 / riscv32i	223.97	256.42 (+14.5%)
nangate45 / swerv	1040.24	1039.75 (-0.0%)
sky130hs / jpeg	265.45	270.01 (+1.7%)

Setup Violation Count

Design	Master	This PR
asap7 / cva6	43	73 (-69.8%)
ihp-sg13g2 / riscv32i	1059	1056 (+0.3%)
nangate45 / swerv	1635	1174 (+28.2%)
sky130hs / jpeg	416	337 (+19.0%)

[maliberty]

Codex:

• Correctness: The phase 1 recovery path clears NesterovPlace’s divergence count, but leaves each NesterovBase::isDiverged_ set. Phase 2 detects that flag again and can throw, so the intended recovery does not work.
• GPU correctness: Filler redistribution changes host coordinates after the GPU context has been built, without syncing the new coordinates to that context. The GPU placement loop can therefore start from the old filler positions.
• Testing: The PR updates an existing golden log but adds no regression test for phase 1 divergence, phase 2 recovery, or the GPU path. Those paths need targeted coverage.

[maliberty]

Claude:

== Priority 1 (correctness) ==

• src/gpl/src/nesterovPlace.cpp guardIncrementalDensityPenalty / applyDensityPenaltyFactor
  bug: the "escalation" doesn't multiply the current penalty. It resets densityPenalty_
  to (wlGradSum/densityGradSum) * factor. Each iteration already multiplies the penalty
  by phiCoef (nesterovBase.cpp:4361), so after N iterations the running penalty can be
  far above ratio*2^k. A "2x escalation" can then cut the penalty by orders of magnitude.
  The golden file shows this at the start of phase 2: phase 1 ends at 2.49e-01 and
  phase 2 restarts at 1.66e-07. Should this be densityPenalty_ = kGrowthRatio, or
  max(current, ratiofactor)?
• same function question: it has no patience. Once overflow sits above
  best+0.005, it doubles on every iteration, so it can reach 1024x in 10 consecutive
  iterations and then stop for good (the retry cap is never reset). Is that intended?
  The "guard-parameter sweep" the comments mention isn't in the PR. Which designs was it run on?
• src/gpl/src/replace.cpp try/catch around doNesterovPlace
  • It catches every std::runtime_error, not only divergence. Any other logger->error
    from inside the placer (timing-driven/rsz, GPL-103, ...) also gets swallowed and
    phase 2 runs anyway.
  • The [ERROR GPL-0307] line has already been printed by the time the catch runs, so
    the log shows an ERROR and the run then continues. Flows and CI that grep for
    ERROR will be misled. Having np_->doNesterovPlace return a divergence status
    would be cleaner than using exceptions for control flow.
  • question: this path only throws when no snapshot was saved. Phase 2 then starts
    from the diverged coordinates, which updateDb() has already written. Are they
    guaranteed finite and sane?
• replace.cpp: np_->getAverageOverflow() bug: if initNesterovPlace() returns false
  (total_placeable_insts_ == 0), doNesterovPlace returns 0 and np_ is still null,
  so this dereferences null. Master never touched np_ here. Add a guard.
• replace.cpp phase 1: undocumented behavior changes. uniformTargetDensityMode = true
  is removed for phase 1, and so is the max(overflow, 0.2) floor. Phase 1 now targets
  the user's overflow, not a fixed 0.1 as the description says. Intentional?
• nesterovBase.cpp redistributeFillerCells: num_fillers - assigned is size_t. Clamp
  it, or assert assigned <= num_fillers, so floating-point rounding can never wrap it
  into partial_sort out of range. (nit, unlikely)
• redistributeFillerCells: free capacity subtracts instPlacedArea. That includes the
  new movable cells at their CG-clumped initial positions, so fillers are kept out of
  space those cells are about to vacate. Was that considered?
  == Priority 2 (QoR) ==
• question: this changes incremental global placement everywhere (repair_design /
  timing-driven re-place in ORFS). What QoR and runtime data do you have from real
  designs (ORFS CI / metrics)? The only evidence is one small odb test.
• That test is a warning sign for phase 1. Locked cells hold overflow at 0.388, so
  chasing 0.1 can't converge. It burns all 600 iterations (2x the runtime) and phase-1
  HPWL grows 71 -> 253. Final GP HPWL improves (169 -> 149), but legalization gets
  worse (avg displacement 1.2 -> 1.5u, max 2.7 -> 3.4u, delta HPWL 6% -> 8%). The old
  0.2 cap / 300 iterations existed to avoid exactly this runaway.
• Skipping phase 2 when phase 1 converges means previously placed cells never move.
  Fine if overflow is met, but worth confirming on congested designs.

== Priority 3 (testing) ==

• No new tests (the PR's own checklist leaves "included tests" unchecked). The
  divergence-recovery path, the penalty guard (GPL-193 never shows up in the golden
  file), and the "not enough free capacity" warning (GPL-332) are all untested.
  The only coverage is the regenerated replace_hier_mod1.ok.
• Bazel flow metrics_limits will probably move. Check the ORFS/Jenkins metrics run.

[LucasYuki]

@maliberty, I fixed the issues that Claude and Gemini.
Although there are some changes in the code, they were all corner-case fixes, the placement results are still the same for all tested designs.
Since it's a lot of small things, I asked Claude to write the response and summarize the fixes.

Fixed (fc119efc60, e579d7cdd0, 5efd57bc12, and a follow-up test commit):

• Phase 1 recovery doesn't actually recover (Codex). Confirmed — clearDivergence() only reset NesterovPlace's own num_region_diverged_/divergeMsg_/divergeCode_, never the per-region NesterovBase::isDiverged_ that checkDivergence() returns. Since the throw path specifically hits when no snapshot was saved (so revertToSnapshot(), the only other place that clears it, never runs), phase 2's first check saw the stale flag and re-threw immediately. NesterovBase::clearDivergence() now resets it too, called from NesterovPlace::clearDivergence() for every region. Added nesterov_divergence_test, a gtest that reproduces this with a real numerical divergence (a clustered-cell layout with an unreachable overflow target, so densityPenalty_ genuinely overflows a float and [ERROR GPL-0305] fires for real) rather than hand-setting the flag — it fails on the pre-fix code and passes after.

• GPU coords stale after filler redistribution (Codex). Confirmed — redistributeFillerCells() wrote the new filler positions to the host vectors only, with no nb_device_ctx_->syncCoordsToDevice(...) call, unlike revertToSnapshot(). Added the matching sync under #ifdef ENABLE_GPU. Couldn't exercise this branch directly (no GPU in this environment), but it's gated by the same nb_device_ctx_ check the rest of the GPU code uses.

• Catch-all std::runtime_error swallows non-divergence errors, and the ERROR line is already logged by the time the catch runs (Claude). Both confirmed, same root cause: Logger::error() always logs at err level and then throws a plain std::runtime_error, so catch (const std::runtime_error&) around phase 1 couldn't tell a real divergence apart from, say, a resizer/timing-driven failure. Rather than string-matching the exception message, we added NesterovPlace::setAllowDivergenceRecovery(bool) / divergedLastRun(): when phase 1 sets this, a divergence is reported via log_->warn(...) and a normal return instead of log_->error(...) + throw. doIncrementalPlace() now checks divergedLastRun() instead of catching anything — a genuine error still throws and propagates normally, and it switches the flag back off before phase 2 (which has no further fallback, so a divergence there should still fail loudly, unchanged from before).

• getAverageOverflow() null-deref if initNesterovPlace() returns false (Claude). Confirmed. Rather than guard every later use, doIncrementalPlace() now exits early (GPL-0139) as soon as total_placeable_insts_ == 0 is known — before phase 1 even starts — so np_ is guaranteed non-null for the rest of the function.

• redistributeFillerCells(): num_fillers - assigned is size_t with no clamp (Claude, nit). Confirmed exploitable in principle (floating-point rounding across many bins could push assigned one over num_fillers, underflowing the subtraction and sending partial_sort's end iterator far out of bounds). Clamped assigned = std::min(assigned, num_fillers) before the subtraction.

• No end-to-end test for phase1_diverged through Replace::doIncrementalPlace() (Claude, Priority 3). Added incremental03: reuses incremental02.def with -max_phi_coef 50 to push the density-penalty escalation hard enough to genuinely diverge (GPL-0305) within phase 1's 600-iteration cap — fully deterministic (checked across repeated runs). It's a passfail test rather than a golden-log comparison: this scenario drives the solver into a numerically chaotic regime by design, so the exact iteration-by-iteration numbers aren't bit-reproducible across toolchains (observed to differ between builds) even though the qualitative log messages are. It captures the run's log via with_output_to_variable and checks fixed substrings — [WARNING GPL-0305] and [WARNING GPL-0195] present, [ERROR GPL-0305] absent. Verified it's load-bearing by temporarily reverting to the pre-fix catch-all code and confirming the test fails with exactly the old signature ([ERROR GPL-0305], old message format).

Confirmed, no change needed:

• Phase 1 dropping uniformTargetDensityMode/the max(overflow, 0.2) floor — intentional.
• Bazel metrics_limits — checked against the Jenkins/ORFS run, it's fine.
• guardIncrementalDensityPenalty's escalation resetting instead of multiplying (Claude). Re-examined this one — it's intentional, not a bug. The concern was that updateDensityPenaltyFromRatio(factor) recomputes densityPenalty_ from the current wireLengthGradSum_/densityGradSum_ ratio instead of multiplying the running value, and that a "2x escalation" can look like it cuts the penalty by orders of magnitude in the log. But that's the point: it recalibrates against the design's current force balance rather than compounding on top of whatever the unrelated phiCoef mechanism (which tracks HPWL trend, not overflow) had drifted the penalty to. The escalation ladder (current_factor doubling: 2x, 4x, 8x... up to 1024x at the 10-retry cap) applied against that fresh ratio each time it's called is a clean, bounded, predictable response specifically to overflow regressing, separate from whatever phiCoef is doing for HPWL. One real limit worth flagging: retries never resets within a run, so after 10 escalations the guard goes permanently silent for the rest of that doNesterovPlace() call if overflow keeps regressing past that point — given kMaxRetries is a deliberate, fixed cap, that reads as an intentional safety bound rather than an oversight.

Priority 2 (QoR) — here's the real data:

Real before/after ORFS output for the four designs this PR actually touches (confirmed separately that the other 63 of 68 common designs produce byte-identical 6_report.json, so there's zero QoR impact anywhere else — and rechecked the full logs_incremental tree again after new designs/platforms were added since; still exactly these same four trigger incremental placement at all).

Peak overflow reached during global placement (lower is better):

Design	Master max overflow	This PR max overflow
asap7/cva6	0.7897	0.1078
ihp-sg13g2/riscv32i	0.9007	0.1199
nangate45/swerv	0.8272	0.1014
sky130hs/jpeg	0.7772	0.1585

Final timing, from 6_report.json (positive % = improvement for every column except the violation count, where positive = fewer violations):

Design	Setup WS	Setup TNS	Hold WS	Fmax	Setup Viol. Count
asap7/cva6	-43.22 → -23.64 (+45.3%)	-1249.2 → -848.8 (+32.1%)	24.961 → 24.233 (-2.9%)	1049.08 → 1071.07 (+2.1%)	43 → 73 (-69.8%)
ihp-sg13g2/riscv32i	-2.46 → -1.90 (+22.9%)	-2108.3 → -1730.2 (+17.9%)	0.233 → 0.233 (+0.0%)	223.97 → 256.42 (+14.5%)	1059 → 1056 (+0.3%)
nangate45/swerv	-0.65 → -0.62 (+3.6%)	-447.9 → -239.8 (+46.5%)	0.016 → 0.011 (-32.2%)	1040.24 → 1039.75 (-0.0%)	1635 → 1174 (+28.2%)
sky130hs/jpeg	-0.27 → -0.20 (+23.8%)	-38.5 → -17.6 (+54.3%)	0.193 → 0.181 (-6.2%)	265.45 → 270.01 (+1.7%)	416 → 337 (+19.0%)

Summary: setup slack/TNS improves on all four, hold slack is roughly flat, fmax is flat-to-slightly-better, and setup violation count improves on three of four — cva6 is the one regression there (69.8% more setup violations) despite its WS/TNS/fmax all improving, worth calling out explicitly since it cuts against the trend.

Separately, asap7/cva6 is also the one design here that hits phase 1's 600-iteration cap (GPL-1001 ... iteration 606 vs master's 407) without reaching its target overflow — it takes the phase1_missed_target path into phase 2 rather than converging cleanly in phase 1 like the other three. None of the four actually diverge (phase1_diverged is false in all of them), so the correctness fixes in this round don't change any of these numbers — this is the steady-state QoR picture for the feature as designed.

On the odb test's "locked cells can't converge" warning sign: dug into the actual legalization numbers in replace_hier_mod1.ok rather than just the displacement/delta% figures. delta HPWL is legalization's relative increase over its own starting GP HPWL, and the two runs start from different GP HPWL (169 vs 149). Once compounded back to absolute numbers, legalized HPWL is 179.4u (master) vs 160.1u (this PR) — a real ~11% improvement, not a regression, despite the higher relative displacement (1.2→1.5u avg, 2.7→3.4u max) and delta% (6%→8%). So the legalization-quality argument doesn't hold up on this test; what's left is a real but narrower concern — phase 1 still burns its full 600-iteration budget and its own interim HPWL still gets worse mid-run (71→253) chasing a target the locked cells make structurally unreachable, which is wasted runtime even though it doesn't harm final quality here. Whether that alone justifies reinstating an early-exit/floor is a judgment call, not something the data forces.

Still open — acknowledging rather than claiming fixed:

• Free-capacity calc in redistributeFillerCells() counts new cells at their pre-spreading CG positions (Claude). Still open — a design tradeoff (one-shot warm-start heuristic vs. steady-state accuracy) rather than a clear bug. Would like your take on whether that's acceptable as-is.
• Diverged coordinates' finiteness (Claude's question on whether updateDb() writing diverged coordinates is safe) — the signaling fix doesn't change what state phase 2 inherits, only how the transition between phases is reported. Still an open question.

[maliberty]

@gudeh merge when you are satisfied and approve