rsz: Prevent hold repair from buffering internal output arcs by openroad-ci · Pull Request #10236 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-04-23T06:34:50Z

Summary

asap7 full adder liberty cell `` has a weird output-to-output timing arc.
Root-cause: Due to the weird timing arc defined in .lib, the timing edge traversal to find load cell instances with a driver pin picked a wrong load instance, which produced ODB-1200 error.
Solution: Filter internal output pins from RepairHold load collection before calling insertBufferBeforeLoads to avoid detecting wrong load instances.

Root Cause

The failing ASAP7 full-adder instance has multiple output pins. The target repair net is driven by one output pin, but the Liberty timing graph also exposes an output-to-output arc from that driver output to another output pin on the same instance, for example CON -> SN.

Before this fix, hold repair effectively treated every timing-graph fanout as a physical net load:

for each timing edge from path_vertex:
  load_pin = edge.to()->pin();
  load_pins.push_back(load_pin);

insertBufferBeforeLoads(drvr_net, load_pins, ...);

That is wrong for an internal output pin reached through an output-to-output timing arc. In the crash case, SN is not connected to the target driver net, so OpenDB rejects buffering it as a load on that net and reports ODB-1200.

Fix

Filter internal output pins while collecting hold-repair loads. Top-level output ports remain valid loads, but output pins on internal instances are not load pins for the target net.

               sta::Slack fanout_hold_slack = sta_->slack(fanout, min_);
               sta::Pin* load_pin = fanout->pin();
+              if (load_pin == nullptr) {
+                continue;
+              }
+              if (network_->direction(load_pin)->isAnyOutput()
+                  && !network_->isTopLevelPort(load_pin)) {
+                continue;
+              }
               if (fanout_hold_slack < hold_margin) {
                 load_pins.push_back(load_pin);
                 Slacks fanout_slacks;

Testing

clang-format -i src/rsz/src/RepairHold.cc
clang-tidy src/rsz/src/RepairHold.cc -p build --quiet
cmake --build build --target openroad -j 8
ctest --test-dir build -R 'rsz\.repair_hold_multi_output_load\.tcl$' --output-on-failure -j 1
ctest --test-dir build -R 'rsz\.(gcd_resize|repair_hold.*)\.tcl$' --output-on-failure -j 4

ASAP7 full-adder cells expose output-to-output timing arcs such as CON -> SN. RepairHold was treating every timing-graph fanout from a driver vertex as a physical net load, so an internal output pin could be passed to insertBufferBeforeLoads for the driver's net. OpenDB correctly rejects that with ODB-1200 because the output pin is not connected to the net being split. The fix filters internal output pins while preserving top-level output ports as valid loads. A reduced netlist regression exercises the failing post-CTS hold repair path without relying on a saved ODB snapshot. Constraint: ASAP7 liberty includes output-to-output timing groups that OpenSTA represents as graph edges Rejected: Pass loads_on_diff_nets=true | would mask invalid load collection and let repair_hold split unrelated nets Rejected: Saved ODB regression | too brittle for OpenROAD architecture refactors Confidence: high Scope-risk: narrow Directive: Do not treat timing graph fanout as physical net loads without direction/connectivity intent checks Tested: clang-format on src/rsz/src/RepairHold.cc Tested: clang-tidy src/rsz/src/RepairHold.cc -p build --quiet Tested: cmake --build build --target openroad -j 8 Tested: ctest --test-dir build -R 'rsz\.repair_hold_multi_output_load\.tcl$' --output-on-failure -j 1 Tested: ctest --test-dir build -R 'rsz\.(gcd_resize|repair_hold.*)\.tcl$' --output-on-failure -j 4 Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

gemini-code-assist

Code Review

This pull request addresses a bug in hold timing repair where internal output pins of multi-output cells were incorrectly treated as load pins. The changes introduce filtering for these pins, add safety checks for empty load sets, and simplify the driver net lookup process. A new regression test is provided to ensure correct behavior. The review feedback suggests a more robust pin filtering condition—checking for the absence of an input direction rather than the presence of an output direction—to avoid incorrectly skipping valid inout pins.

gemini-code-assist · 2026-04-23T06:36:45Z

+              if (network_->direction(load_pin)->isAnyOutput()
+                  && !network_->isTopLevelPort(load_pin)) {
+                continue;
+              }


The current check using isAnyOutput() will filter out internal inout pins, which are valid load pins. To correctly filter only pure output pins (which are the ones causing the issue with internal timing arcs), it is safer to check if the pin lacks an input component.

Suggested change

if (network_->direction(load_pin)->isAnyOutput()

&& !network_->isTopLevelPort(load_pin)) {

continue;

}

if (!network_->isTopLevelPort(load_pin)

&& !network_->direction(load_pin)->isAnyInput()) {

continue;

}

github-actions · 2026-04-23T06:39:41Z

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

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-04-23T07:27:59Z

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

jhkim-pii · 2026-04-23T12:29:33Z

@codex review

chatgpt-codex-connector · 2026-04-23T12:35:00Z

Codex Review: Didn't find any major issues. 👍

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

maliberty · 2026-04-23T23:38:20Z

This is not a good test. It is not minimal in data size nor in command scope (running a complete flow from .v will be susceptible to changes in all the previous commands results).

Reduced the test case.

This concern is still not full addressed. It depends on initialize_floorplan and detailed_placement. It would be more natural to have an input def file that is already placed and drop the .v. Look at most of the other tests.

I assume this is AI generated so perhaps some guidance in agents.md would be helpful.

Reduced the new test case again.

Add OR & ORFS PRs.

For better OR test case creation: doc: Add a new guide for creating issue reproduction test case #10269

Enable OR AGENTS.md discovery when the AI agent is executed from the ORFS root: doc: Add AGENTS.md OpenROAD-flow-scripts#4193

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-04-24T01:09:35Z

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

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-04-27T01:00:12Z

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

jhkim-pii · 2026-04-23T06:36:35Z

 #include "sta/Graph.hh"
 #include "sta/GraphClass.hh"
-#include "sta/GraphDelayCalc.hh"
-#include "sta/InputDrive.hh"


jhkim-pii · 2026-04-23T06:36:49Z

-  } else {
-    drvr_dbnet = static_cast<odb::dbITerm*>(drvr_db_pin)->getNet();
-  }
+  odb::dbNet* drvr_dbnet = db_network_->findFlatDbNet(drvr_pin);


Minor refactoring

jhkim-pii · 2026-04-23T06:37:24Z

+              if (network_->direction(load_pin)->isAnyOutput()
+                  && !network_->isTopLevelPort(load_pin)) {
+                continue;
+              }


This is the fix.
Output pin cannot be a load pin.

jhkim-pii · 2026-04-24T03:43:40Z

Reduced the test case.

jhkim-pii · 2026-04-27T01:37:45Z

Reduced the new test case again.

Add OR & ORFS PRs.

For better OR test case creation: doc: Add a new guide for creating issue reproduction test case #10269

Enable OR AGENTS.md discovery when the AI agent is executed from the ORFS root: doc: Add AGENTS.md OpenROAD-flow-scripts#4193

openroad-ci assigned jhkim-pii Apr 23, 2026

github-actions Bot added the size/XL label Apr 23, 2026

gemini-code-assist Bot reviewed Apr 23, 2026

View reviewed changes

jhkim-pii added 2 commits April 23, 2026 16:09

Apply review feedback

005c3c6

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Adjust repair_hold_multi_output_load.tcl test case

83aab97

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii requested a review from maliberty April 23, 2026 12:29

maliberty reviewed Apr 23, 2026

View reviewed changes

Reduce repair_hold_multi_output_load.tcl

fb939e9

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions Bot added size/S and removed size/XL labels Apr 24, 2026

jhkim-pii requested a review from maliberty April 24, 2026 03:43

rsz: Use placed DEF for multi-output hold test

9ed4780

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii reviewed Apr 27, 2026

View reviewed changes

maliberty approved these changes Apr 27, 2026

View reviewed changes

maliberty merged commit 940703b into The-OpenROAD-Project:master Apr 27, 2026
16 checks passed

maliberty deleted the secure-fix-repair-hold branch April 27, 2026 17:36

jhkim-pii mentioned this pull request Apr 28, 2026

rsz: bail repair_timing on obviously-futile designs #10248

Closed

Uh oh!

Conversation

openroad-ci commented Apr 23, 2026

Summary

Root Cause

Fix

Testing

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 23, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 23, 2026

Uh oh!

github-actions Bot commented Apr 23, 2026

Uh oh!

jhkim-pii commented Apr 23, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 23, 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!

github-actions Bot commented Apr 24, 2026

Uh oh!

github-actions Bot commented Apr 27, 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants