[CTS] Fix cloned ICG location collision in mapLocationToSink_

[openroad-ci]

Summary

• When cloneClockGaters() places multiple ICG clones at the same sinksBbox center, the single-valued mapLocationToSink_ map in HTreeBuilder silently overwrites earlier entries, disconnecting ICGs from the clock tree.
• Add resolveLocationCollision() to ensure each clone gets a unique position by tracking all occupied instance positions and shifting colliding clones by site width.
• Physical overlap is resolved by the subsequent detailed_placement step in the ORFS flow.

Test plan

• New colocated_sinks regression test that reproduces the bug with two ICGs whose sink bounding boxes share the same center
• Full CTS regression (52/52 pass)
• Full OpenROAD regression (all pass, 0 failures)
• CI

[gemini-code-assist]

Code Review

This pull request addresses a bug where multiple ICG clones placed at the same location would cause an entry in mapLocationToSink_ to be overwritten, effectively disconnecting one of the ICGs. The fix introduces a collision resolution mechanism that tracks occupied positions and shifts new clones to a unique location if a collision is detected. The changes look good and are supported by a new regression test. I have a couple of suggestions: one to improve performance by avoiding redundant work, and another to prevent a potential infinite loop in the collision resolution logic.

[gemini-code-assist]

There's a potential for an infinite loop here. If block_->getRows() is not empty but block_->getRows().begin()->getSite()->getWidth() returns 0, shift will be set to 0. If a location collision occurs, the while loop on line 1205 will never terminate because cloneLoc will not be updated.

To make this more robust, you should ensure shift is always a positive value.

  int shift = 1;
  if (!block_->getRows().empty()) {
    const int site_width = block_->getRows().begin()->getSite()->getWidth();
    if (site_width > 0) {
      shift = site_width;
    }
  }

[jhkim-pii]

Resolved by removing the use of getWidth().

[gemini-code-assist]

This function iterates over all instances in the block to populate occupiedPositions. Since cloneClockGaters is called for each clock net within populateTritonCTS, this can lead to significant performance degradation on designs with many clock nets and instances, as the instance list is traversed repeatedly.

To optimize this, I recommend creating the occupiedPositions set once in populateTritonCTS before the clock net loop and passing it to the overloaded cloneClockGaters function. This would make the population of existing instance locations a one-time operation.

References

1. When analyzing code within a loop, consider the entire loop structure. Operations at the beginning of the loop can affect the logic within the loop body, potentially making similar calls inside the loop redundant.

[github-actions]

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

[github-actions]

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

[jhkim-pii]

Fixes #9816

Failure Sequence

1. cloneClockGaters() / findLongEdges() — Two or more ICG clones are placed at the same sinksBbox.center() because their sinks are arranged symmetrically around an identical center point.
2. HTreeBuilder::preSinkClustering() — Populates mapLocationToSink_[normLoc] = &sink. Since the map is single-valued (std::map<Point<double>, ClockInst*>), the second ICG at the same position silently overwrites the first entry.
3. HTreeBuilder::createClockSubNets() — Looks up sinks via mapLocationToSink_[loc]. The overwritten ICG is never found, so it is not connected to any CTS buffer driver. Its clock input pin remains disconnected.
4. Kepler Formal (LEC) — Detects the unconnected clock input pin (e.g., .A2 on AND-based gating cells in mempool_group) as a connectivity mismatch between RTL and netlist.

// Connected clone (correct)
AND2_X1 \clone_11_..._11727_
    (.A1(\..._WORD_ITER[2].CG_Inst.clk_en ),
     .A2(\clknet_leaf_164_..._06692_ ),
     .ZN(\..._WORD_ITER[2].CG_Inst.clk_o ));

// Disconnected clone (BUG)
AND2_X1 \clone_10_..._11727_
    (.A1(\..._WORD_ITER[2].CG_Inst.clk_en ),
     .ZN(\..._WORD_ITER[2].CG_Inst.clk_o ));
// NOTE: .A2 missing — clock input is floating

Root Cause

cloneClockGaters() in TritonCTS.cpp clones ICG (Integrated Clock Gating) cells and places each clone at the center of its sink bounding box (sinksBbox.center()). When two or more ICGs drive sinks arranged symmetrically around the same center point, all clones are placed at the identical location.

Later, HTreeBuilder::preSinkClustering() populates mapLocationToSink_ — a std::map<Point<double>, ClockInst*> — keyed by each sink's position. Since this is a single-valued map, when two ICG clones share the same position, the second insert silently overwrites the first. The overwritten ICG is then lost from the clock tree: its clock input pin is never connected to a CTS buffer driver, leaving it disconnected.

This manifests as missing clock connections on ICG clock input pins in designs with symmetrically placed gated clock sinks (e.g., mempool_group).

Solution

1. resolveLocationCollision() — a new private method in TritonCTS (named to match HTreeBuilder::resolveLocationCollision() which solves the same problem for CTS buffers). After placing a clone at sinksBbox.center(), this method checks an occupiedPositions set and shifts the clone by one site width until a unique position is found.

2. occupiedPositions set — seeded in the cloneClockGaters() wrapper with all existing instance positions in the block, then passed by reference through the recursive cloning. This prevents collisions against both other clones and pre-existing placed cells.

3. No DPL dependency — physical legalization (site alignment, overlap removal) is handled by the detailed_placement call that follows CTS in the ORFS flow (cts.tcl line 49). The fix only ensures unique positions for mapLocationToSink_ correctness.

[github-actions]

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

[jhkim-pii]

@arthurjolo I found cloned ICG cell location collision, which is similar to the previous issue #9313 .
Could you please review this PR?

[jhkim-pii]

@arthurjolo I found this during the debugging.

[ Current: Allow cell overlaps during CTS + DPL ]

• Cloned ICGs and new clock buffers are placed during clock_tree_synthesis.
• But they can have cell overlap during clock_tree_synthesis.
• After clock_tree_synthesis, detailed_placement is called to resolve cell overlaps.
• ISSUE: The cell displacement after detailed_placement can increase clock skew.

[ Proposed: Just-in-time legalization when a new cell is inserted during CTS ]

• If incremental legalization is executed whenever a new cell is inserted during clock_tree_synthesis, no detailed_placement command call is required and there will be no clock skew degradation by detailed_placement.
• Does this make sense?
• Or, is there a difficulty in performing the incremental legalization right after inserting a new cell during CTS?

[arthurjolo]

Hi Jaehyun, thanks for looking into this issue. Do you have a testcase where the cloning of the ICGs generate this overlap? When I made this feature I didn't take into into account the possibility of 2 clones having the same position.

[arthurjolo]

@arthurjolo I found this during the debugging.

[ Current: Allow cell overlaps during CTS + DPL ]

* Cloned ICGs and new clock buffers are placed during `clock_tree_synthesis`.- But they can have cell overlap during `clock_tree_synthesis`.- After `clock_tree_synthesis`, `detailed_placement` is called to resolve cell overlaps.- ISSUE: The cell displacement after `detailed_placement` can increase clock skew.

[ Proposed: Just-in-time legalization when a new cell is inserted during CTS ]

* If incremental legalization is executed whenever a new cell is inserted during `clock_tree_synthesis`, no `detailed_placement` command call is required and there will be no clock skew degradation by `detailed_placement`.

* Does this make sense?

* Or, is there a difficulty in performing the incremental legalization right after inserting a new cell during CTS?

Regarding this comment, it does make sense DPL can cause some skew, but usually this skew is very minimal. The only case where legalization caused some meaningful impact was when we placed a buffer on top of macro block, in this case the buffer had a big change in its position an caused a big skew. Now we have obstruction aware to avoid that issue, but it only considers collisions with macros, for collisions other cells the change in position by DPL is usually very small.

I believe implementing an incremental legalization in CTS is not worth it, I believe the overhead is to big and it won't bring that many improvements.

@maliberty and @precisionmoon do you have different opinion on this?

[github-actions]

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

[jhkim-pii]

Hi Jaehyun, thanks for looking into this issue. Do you have a testcase where the cloning of the ICGs generate this overlap? When I made this feature I didn't take into into account the possibility of 2 clones having the same position.

The way to reproduce the issue w/ nangate45/mempool_group is described in the issue description (#9816).

OpenROAD | e03452289af96d9f3e254591261bdb138f8a0c40
ORFS | 4ba75de61d1b4dcdf1009350a1884988e5e7b3ec

flow$ make DESIGN_CONFIG=./designs/nangate45/mempool_group LEC_CHECK=1

[github-actions]

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

[arthurjolo]

Code changes look fine.

Just the comment regarding the shift unit seems outdated.

And one minor suggestion on calling setLocation

[arthurjolo]

The comments mentions a shift by the site width but I only see it beeing set to 1DBU.

[jhkim-pii]

Oops. Good catch. Thanks. I will update the comment.

[arthurjolo]

We are calling clone->setLocation(x, y) here and at the end of resolveLocationCollision(...) as well.

To avoid going throw all the callbacks of changing a cell location twice we can pass to resolveLocationCollision(...) the position and only call clone->setLocation(x, y) in there. Since we are not creating many clones I don't think there will be a meaningful impact on runtime, but having many changes on DB have caused me problems in the past.

[jhkim-pii]

Done

[jhkim-pii]

No ORFS QoR impact by this fix because the collision happens rarely.

• Dashboard - Base vs Test
• nangate45/swerv_wrapper QoR impact is likely to be caused by
  • [Synthesis] nangate45/swerv_wrapper QoR fluctuation due to Yosys abc_new nondeterminism OpenROAD-flow-scripts#4056

[github-actions]

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

[maliberty]

Add to BUILD

[github-actions]

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

[github-actions]

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