Repository navigation
rsz: detect cross-clock-domain hold violations on clock-as-data endpoints (#8075) - #10718
saurav-fermions wants to merge 2 commits into
Conversation
…ints (The-OpenROAD-Project#8075) findHoldViolations excluded every endpoint for which Sta::isClock() is true. isClock() reports any pin in a clock network's combinational fanout, which also includes register data (D) pins that sample a signal sourced from a clock -- e.g. a clock used as data that crosses into a different sampling-clock domain. Those D pins are legitimate hold endpoints, so repair_timing -hold silently reported "No hold violations found" while a real hold violation remained (issue The-OpenROAD-Project#8075). Refine the exclusion to skip only true clock-tree pins: pins that are on the clock network AND carry no timing check (!Vertex::hasChecks()). Register data pins that hold a setup/hold check are kept, so cross-clock -domain hold violations are detected and repaired. Clock-tree pins are still excluded as before. Add regression test repair_hold_clock_as_data where clk1 is used both as a clock (for r1) and as data into r2/D (r2 clocked by clk2). On the unfixed code RSZ reports "No hold violations found" and r2/D stays at -0.59; with the fix the endpoint is detected and repaired to >= 0. Registered in both CMake and Bazel. Signed-off-by: Saurav Singh <saurav.singh@fermions.co>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses OpenROAD issue #8075 by updating RepairHold::findHoldViolations to ensure that register data pins sampling a signal sourced from a clock (cross-clock-domain endpoints) are not incorrectly skipped during hold repair. This is achieved by only excluding clock tree endpoints that do not have timing checks. Additionally, a new integration test (repair_hold_clock_as_data) has been added to verify this behavior. I have no feedback to provide as there are no review comments.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 21 days if no further activity occurs. Remove the |
github-actions
Bot
added
the
Stale
|
Not stale from my side — still want this one. Recap for anyone picking it up: This fixes #8075 — It has a gemini review pass and no outstanding human change-requests. I'll rebase it onto current master and re-verify the rsz suite so it's ready for review — happy to get a maintainer's eyes on it whenever convenient. |
github-actions
Bot
removed
the
Stale
|
Not stale from my side — still want this one. Recap for anyone picking it up: This fixes #8075 — It has a gemini review pass and no outstanding human change-requests. I'll rebase it onto current master and re-verify the rsz suite so it's ready for review — happy to get a maintainer's eyes on it whenever convenient. |
|
@jhkim-pii please review |
|
@saurav-fermions It looks like this PR enables clock network for hold fixing, which may cause the side-effect of hold-buffer added clock tree (clock latency of certain branches can be affected). I think repair hold can break the clock tree because it does not specifically consider the clock network. |
|
@saurav-fermions needs a reply from you |
…paths With clock-as-data endpoints now visible to repair_timing -hold, the worst hold path to such an endpoint starts at the clock source rather than at a register clk->Q arc, so repairEndHold walks drivers that are part of the clock tree. A driver whose net also feeds clock sinks with negative (propagated) hold slack would get a hold buffer in front of those clock sinks, changing their clock latency. Compute the set of clock-network pins that transitively drive a clock sink (register/check clock pins, clock output ports) and skip those drivers when choosing a hold-buffer insertion point. Clock-network pins that only feed data pins are still eligible, so the clock-as-data hold violation is repaired on the data side. Add repair_hold_clock_as_data_clk_tree: clk1 drives r1/CK via i1 and r2/D via dbuf. Without this change the hold buffer is inserted on the clk1 net driving i1/A and r1/CK clock arrival moves from 0.018 to 0.373; with it the clk1 net and r1/CK arrival are unchanged and both hold endpoints are repaired. repair_hold_clock_as_data.ok is updated because its buffer now goes after dbuf instead of on the clk1 net. Signed-off-by: Saurav Singh <saurav.singh@fermions.co>
|
@jhkim-pii you were right, thanks for catching it, and sorry for the slow reply @maliberty. A clock-as-data hold path has no clk->Q arc, so it starts at the clock source. Every clock-tree driver up to the D pin became a place where Fix in
One limitation: if a clock net drives a clock sink and the D pin directly, that endpoint is now left unrepaired rather than buffered on the clock net. I haven't run it on the original #8075 design. |
|
@saurav-fermions I approved your change. Could you please resolve the merge conflict? |
|
Replaced by #12091 to resolve the conflict |
Summary
RepairHold::findHoldViolationsskipped endpoints whose pin is a clock pin, so a hold violation on a clock-as-data endpoint was never repaired. This detects cross-clock-domain hold violations on clock-as-data endpoints.Type of Change
Impact
Hold violations on clock-as-data endpoints are now detected and repaired.
Verification
ctest -R '^rsz\.'239/239; real fail/pass viarepair_hold_clock_as_data.Related Issues
Fixes #8075
Developed with SAIGE, Fermions' autonomous RTL/EDA debugging agent; root-caused, tested, and signed off by the submitter (@saurav-fermions).