rsz: clamp buffer locations to core area during insertion by openroad-ci · Pull Request #10141 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-04-15T12:48:31Z

Summary

RSZ-inserted buffers could land outside the core area, in the die-core gap where no placement rows exist, causing downstream DPL legalization failures. Clamp buffer coordinates to the core bounding box at the point of insertion.

Root Cause

The buffers in question are wire repeaters produced by repair_design (and similarly by repair_setup / repair_hold rebuffering). The insertion mechanism:

RSZ builds a Steiner tree over the driver pin + all load pins of a net (SteinerTree::findSteinerPoints).
isTopLevelPort(load_pin) returns true for BTerms, so top-level ports become Steiner terminals. Their coordinates come from dbBTerm::getFirstPinLocation() — which returns the die-boundary pin location, not a core-side coordinate.
When a wire segment from a core-side point to a die-edge BTerm exceeds max_wire_length, RepairDesign::repairNet inserts a repeater by interpolating a point along the segment:
```
buf_x = to_x + d * dx    // d ∈ (0, 1]
buf_y = to_y + d * dy
```
If the segment endpoint is at the die edge, the interpolated point lands inside the die-core gap — the band between die boundary and core row area.
setLocation(buf_inst, {buf_x, buf_y}) was called without clamping — buffer placed outside core.
DPL cannot legalize: no placement row exists at those coordinates.

Verification

Reproduced on a customer design — see The-OpenROAD-Project-private/OpenROAD-flow-scripts#1622 for details. Wire repeaters inserted during the place-resize stage were observed outside the core, all traced to die-edge BTerms feeding the Steiner-point interpolation.

Fix

clampLocToCore() (Resizer.cc, Resizer.hh):

Returns (x, y) clamped into [core.xMin, core.xMax - cell.width] × [core.yMin, core.yMax - cell.height]
std::max guard for the upper bound handles the edge case where the core is smaller than the cell (so std::clamp is safe — requires hi >= lo)

setLocation() applies the clamp and emits [INFO RSZ-0077] {instance_name} clamped to core ({orig_x}, {orig_y}) -> ({new_x}, {new_y}) whenever a buffer is moved — per-buffer detail, always on, instance name lets you grep the DEF directly.

Two insertion paths are both covered:

setLocation() path (repair_hold, repair_setup, make_buffer) — clamp inside setLocation()
ODB insertBuffer* path (repair_design wire repeaters) — insertBufferPostProcess(buffer_inst, loc) calls setLocation after ODB places the buffer, re-placing with the clamped coordinate if needed

Why RSZ-layer (not ODB-layer) clamping

Tempting alternative: clamp inside dbInsertBuffer::placeBufferAtLocation() (single ODB site). Rejected because:

"Core area" is a placement/floorplan concept; ODB is the geometry DB layer. Adding core-awareness to ODB is a layering violation.
dbInsertBuffer is used by non-RSZ callers (e.g., swap_pins_dont_touch) that don't necessarily want core-clamping.
RSZ-layer clamping keeps the policy co-located with the decision-maker and still hits every path through one helper.

Testing

New test: repair_design_outside_core

200×200 μm die with tight core (10, 10)–(90, 90) μm
Driver at core center, 10 loads deliberately placed outside core at x=195000 DBU
set_max_fanout 5 + repair_design -max_wire_length 100 triggers wire-repeater insertion
Asserts all TIMING-source instances remain inside the core post-repair
Log confirms per-buffer clamp actions

Updated golden files (11 tests) — all exercise paths that now emit per-buffer RSZ-0077 info messages:

Test	Why it hits the clamp
`repair_hold2` / `repair_hold8` / `repair_hold11` / `repair_hold12`	Loads at die-bottom pins (y=0) → hold-buffer insertion lands below core
`repair_hold14`	Load placement outside core → hold buffer clamped
`repair_slew4` / `repair_slew12`	Driver cell larger than core on one axis → clamp fires
`repair_wire11`	Long wires to off-core loads → repeaters clamped
`repair_tie5`	Tie-cell insertion near die edge
`upf_aes`	Timing-driven global_placement inserts a repair_design buffer whose top edge extends past core yMax
`repair_design_outside_core`	New test (see above)

Full rsz regression: 195/195 pass.

Type of Change

Bug fix

Impact

All RSZ-inserted buffers now land inside the core. DPL legalization no longer fails on wire repeaters created from die-edge BTerms.
Designs that previously produced out-of-core buffers will see slightly different timing results (wire lengths / parasitics reflect the new, legal locations).
New per-buffer RSZ-0077 info messages replace the prior summary warning — instance name + coordinates, more useful for diagnosis, no debug flag needed.

Verification

Local build passes (./etc/Build.sh).
Relevant rsz regression (195/195) passes.
clang-format, tclfmt, tclint clean.
Commits signed (DCO).

Related Issues

Fixes The-OpenROAD-Project-private/OpenROAD-flow-scripts#1622

gemini-code-assist

Code Review

This pull request replaces the manual buffer-moved-into-core warning with an automatic clamping mechanism using a new clampLocToCore method, and adds a corresponding test case. Feedback highlights that the modified signature of insertBufferPostProcess causes compilation errors at several call sites where references are passed instead of pointers, and misses other sites like makeBuffer. To resolve these errors and improve API usability, it is recommended to revert the signature change and have the method retrieve the buffer's location internally, ensuring consistent clamping for all inserted buffers.

gemini-code-assist · 2026-04-15T12:51:39Z

                            const odb::Point& loc);

-  void insertBufferPostProcess(odb::dbInst* buffer_inst);
+  void insertBufferPostProcess(odb::dbInst* buffer_inst, const odb::Point* loc);


The change to the signature of insertBufferPostProcess to take a pointer const odb::Point* loc introduces several issues:

Compilation Errors: The call sites updated in Resizer.cc (e.g., line 4910) pass a reference loc to this pointer parameter, which will not compile in C++.

Incomplete Updates: Other call sites not included in this diff (such as makeBuffer) will also fail to compile as they now lack the required second argument.

Clamping Policy: Making the location optional via a pointer risks missing clamping for some buffers if nullptr is passed, which contradicts the goal of ensuring all RSZ-inserted buffers land inside the core.

Instead of changing the signature, it is recommended to keep the original void insertBufferPostProcess(odb::dbInst* buffer_inst) and internally call setLocation(buffer_inst, buffer_inst->getLocation()). This ensures all inserted buffers are clamped using their current location without needing to modify any call sites, prioritizing API usability.

Suggested change

void insertBufferPostProcess(odb::dbInst* buffer_inst, const odb::Point* loc);

void insertBufferPostProcess(odb::dbInst* buffer_inst);

References

API usability can be prioritized over strict const-correctness.

gemini-code-assist · 2026-04-15T12:51:39Z


  // Legalize the buffer position and update the design area
-  insertBufferPostProcess(buffer_inst);
+  insertBufferPostProcess(buffer_inst, loc);


This call will fail to compile because loc is a reference (const odb::Point&) but the new signature of insertBufferPostProcess expects a pointer. Reverting the signature change as suggested in the header file would resolve this and simplify the call sites.

insertBufferPostProcess(buffer_inst);

gemini-code-assist · 2026-04-15T12:51:39Z


  // Legalize the buffer position and update the design area
-  insertBufferPostProcess(buffer_inst);
+  insertBufferPostProcess(buffer_inst, loc);


This call will fail to compile because loc is a reference (const odb::Point&) but the new signature of insertBufferPostProcess expects a pointer.

insertBufferPostProcess(buffer_inst);

gemini-code-assist · 2026-04-15T12:51:40Z


  // Legalize the buffer position and update the design area
-  insertBufferPostProcess(buffer_inst);
+  insertBufferPostProcess(buffer_inst, loc);


This call will fail to compile because loc is a reference (const odb::Point&) but the new signature of insertBufferPostProcess expects a pointer.

insertBufferPostProcess(buffer_inst);

gemini-code-assist · 2026-04-15T12:51:40Z

+void Resizer::insertBufferPostProcess(dbInst* buffer_inst,
+                                      const odb::Point* loc)
 {
+  // Clamp to core if the caller provided a target location.  ODB's
+  // insertBuffer* already placed the instance at *loc; setLocation will
+  // re-place it if clamping changed the point.
+  if (loc) {
+    setLocation(buffer_inst, *loc);
+  }


By reverting the signature change and querying the instance's current location, you ensure that clamping is always applied to any buffer undergoing post-processing, regardless of how it was instantiated. This also fixes the compilation errors at the call sites and ensures that methods like makeBuffer also benefit from the clamping logic. This approach prioritizes API usability over the proposed signature change.

void Resizer::insertBufferPostProcess(dbInst* buffer_inst) { // Clamp to core. ODB's insertBuffer* already placed the instance; // setLocation will re-place it if clamping is required. setLocation(buffer_inst, buffer_inst->getLocation()); }

References

API usability can be prioritized over strict const-correctness.

github-actions · 2026-04-15T12:58:22Z

clang-tidy review says "All clean, LGTM! 👍"

minjukim55 · 2026-04-15T14:00:56Z

Scope clarification — the branch now includes commit afb03b614a adding a code comment in insertBufferPostProcess to document the port buffer exception. The PR description below should also be read with this clarification:

Port buffer exception

bufferInputs / bufferOutputs pass loc=nullptr into insertBufferAfterDriver / insertBufferBeforeLoad; insertBufferPostProcess skips the clamp when loc == nullptr. Port buffers are placed by ODB near their BTerm location, which can sit at the die edge outside the core area, and that is by design — a port buffer should stay adjacent to the port it buffers. This matches the pre-existing behavior.

The three paths through insertBufferPostProcess:

setLocation() path (repair_hold, repair_setup, make_buffer) — clamp inside setLocation()
ODB insertBuffer* path with explicit loc (repair_design wire repeaters) — clamp via insertBufferPostProcess(buffer_inst, loc)
Port buffer path (bufferInputs / bufferOutputs, loc=nullptr) — not clamped (by design, see above)

The "Impact" section of the description should be read as: wire repeaters and other loc-aware inserts now land inside the core; port buffers remain at BTerm location (unchanged from prior behavior).

github-actions · 2026-04-15T14:09:31Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-04-16T03:17:08Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-04-16T03:26:48Z

clang-tidy review says "All clean, LGTM! 👍"

jhkim-pii · 2026-04-16T07:13:55Z

+  // ODB's insertBuffer* already placed the instance; re-run through
+  // setLocation so clampLocToCore can pull it back inside the core if
+  // the insertion point landed in the die-core gap.
+  setLocation(buffer_inst, buffer_inst->getLocation());


How about replacing the setLocation() call in the following API?

void dbInsertBuffer::placeBufferAtLocation(dbInst* buffer_inst, const Point& loc, const char* reason) { buffer_inst->setLocation(loc.getX(), loc.getY()); buffer_inst->setPlacementStatus(dbPlacementStatus::PLACED); dlogPlacedBuffer(buffer_inst, loc, reason); }

makeInstance -> setLocation() path don't call placeBufferLocation() so rsz module's clamp() is still needed.

sta::Instance* Resizer::makeInstance(sta::LibertyCell* cell,
const char* name,
sta::Instance* parent,
const odb::Point& loc,
const odb::dbNameUniquifyType& uniquify)
{

OK. I see.

Only Resizer::setLocation() can see the core area (Resizer::core_ attribute), and Resizer::setLocation() cannot be called in odb::dbInsertBuffer::placeBufferAtLocation().

maliberty · 2026-04-16T15:44:17Z

I have no further comments beyond @jhkim-pii

github-actions · 2026-04-22T06:24:52Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-04-22T09:13:21Z

clang-tidy review says "All clean, LGTM! 👍"

minjukim55 · 2026-04-24T05:32:41Z

ORFS : The-OpenROAD-Project/OpenROAD-flow-scripts#4183

jhkim-pii · 2026-04-27T06:18:05Z

+# Tight core: (10, 10) to (90, 90) microns within a 200x200 die.
+initialize_floorplan -site $site \
+  -die_area {0 0 200 200} \
+  -core_area {10 10 90 90}


If the new test case provides repair_design_outside_core.def, this regression can simply read the def file w/o initialize_floorplan call.
How about reducing the test case further?

minjukim55 · 2026-04-16T04:06:56Z

-      y = core_.yMax() - height;
-      buffer_moved_into_core_ = true;
-    }
+  const odb::Point loc = clampLocToCore(pt, db_inst->getMaster());


Main change

minjukim55 · 2026-04-16T04:07:04Z


-  db_inst->setPlacementStatus(dbPlacementStatus::PLACED);
-  db_inst->setLocation(x, y);
+odb::Point Resizer::clampLocToCore(const odb::Point& loc,


minjukim55 · 2026-04-16T04:07:52Z

-void Resizer::warnBufferMovedIntoCore()
-{
-  if (buffer_moved_into_core_) {
-    logger_->warn(RSZ, 77, "some buffers were moved inside the core.");


Removed unnecessary comment (It’s only implemented in code, but it’s not actually written to the log)

minjukim55 · 2026-04-16T04:08:12Z

+---------------------------------------------------------------------
+        0 |     +0.0% |       0 |       0 |             0 |        24
+[DEBUG RSZ-buffer_clamp] fanout1 clamped to core (51780, 1230) -> (51780, 22400)
+[DEBUG RSZ-buffer_clamp] fanout2 clamped to core (1780, 101230) -> (20140, 101230)


dbg msg if enabled

minjukim55 · 2026-04-17T05:42:57Z

+  // ODB's insertBuffer* already placed the instance; re-run through
+  // setLocation so clampLocToCore can pull it back inside the core if
+  // the insertion point landed in the die-core gap.
+  setLocation(buffer_inst, buffer_inst->getLocation());


makeInstance -> setLocation() path don't call placeBufferLocation() so rsz module's clamp() is still needed.

sta::Instance* Resizer::makeInstance(sta::LibertyCell* cell,
const char* name,
sta::Instance* parent,
const odb::Point& loc,
const odb::dbNameUniquifyType& uniquify)
{

Squashed from secure-rsz-instance-outside-core (13 commits) and rebased onto current ORFS master OR pointer (0504146) to isolate the clamp change from unrelated rsz refactors (ServiceRegistry port, OpenSTA sync, dpl-negotiation-fixes, etc.) that landed on master during development. Includes: - clampLocToCore() utility - insertBufferPostProcess folds setLocation with clamp - per-buffer info log replaces summary warning - repair_design_outside_core test - aes_sky130hd metrics limits update Signed-off-by: minjukim55 <mkim@precisioninno.com>

Replace the initialize_floorplan + manual setLocation loop with a read_def -floorplan_initialize call, per Matt's review guidance that test cases should not invoke high-level commands (initialize_floorplan, placement, routing) when those steps are unrelated to the buggy point. Also reduce the load count from 10 to 6 (just over the max_fanout 5 threshold), keeping coverage of both the fanout buffer and wire repeater clamp paths while shrinking the script from ~95 to ~21 lines. The verification loop is removed in favor of relying on the .ok snapshot diff to detect regressions in clamp behavior. Signed-off-by: minjukim55 <mkim@precisioninno.com>

github-actions · 2026-04-27T10:47:39Z

clang-tidy review says "All clean, LGTM! 👍"

maliberty · 2026-04-28T04:05:17Z

+read_verilog repair_design_outside_core.v
+link_design outside_core
+read_def -floorplan_initialize repair_design_outside_core.def


It doesn't matter much for such a small test case but you don't really need the verilog at all. You could just load a def file.

openroad-ci assigned minjukim55 Apr 15, 2026

github-actions Bot added the size/M label Apr 15, 2026

gemini-code-assist Bot reviewed Apr 15, 2026

View reviewed changes

minjukim55 requested review from jhkim-pii and maliberty April 16, 2026 04:08

jhkim-pii reviewed Apr 16, 2026

View reviewed changes

openroad-ci force-pushed the secure-rsz-instance-outside-core branch from 31e0b05 to b9e31bd Compare April 22, 2026 06:15

openroad-ci force-pushed the secure-rsz-instance-outside-core branch from b9e31bd to 5f97307 Compare April 22, 2026 09:04

openroad-ci mentioned this pull request Apr 23, 2026

Update OpenROAD: rsz buffer core-area clamping fix The-OpenROAD-Project/OpenROAD-flow-scripts#4183

Merged

2 tasks

jhkim-pii reviewed Apr 27, 2026

View reviewed changes

minjukim55 reviewed Apr 27, 2026

View reviewed changes

minjukim55 added 2 commits April 27, 2026 17:38

openroad-ci force-pushed the secure-rsz-instance-outside-core branch from 5f97307 to 7ac212c Compare April 27, 2026 10:38

jhkim-pii approved these changes Apr 27, 2026

View reviewed changes

maliberty approved these changes Apr 28, 2026

View reviewed changes

maliberty merged commit 9aeb98b into The-OpenROAD-Project:master Apr 28, 2026
16 checks passed

maliberty deleted the secure-rsz-instance-outside-core branch April 28, 2026 04:05

jhkim-pii mentioned this pull request Apr 28, 2026

rsz: bail repair_timing on obviously-futile designs #10248

Closed

	void insertBufferPostProcess(odb::dbInst* buffer_inst, const odb::Point* loc);
	void insertBufferPostProcess(odb::dbInst* buffer_inst);

Uh oh!

Conversation

openroad-ci commented Apr 15, 2026

Summary

Root Cause

Verification

Fix

Why RSZ-layer (not ODB-layer) clamping

Testing

Type of Change

Impact

Verification

Related Issues

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 15, 2026

Uh oh!

minjukim55 commented Apr 15, 2026

Port buffer exception

Uh oh!

github-actions Bot commented Apr 15, 2026

Uh oh!

github-actions Bot commented Apr 16, 2026

Uh oh!

github-actions Bot commented Apr 16, 2026

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!

maliberty commented Apr 16, 2026

Uh oh!

github-actions Bot commented Apr 22, 2026

Uh oh!

github-actions Bot commented Apr 22, 2026

Uh oh!

minjukim55 commented Apr 24, 2026

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!

github-actions Bot commented Apr 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone