Skip to content

cts: add opt-in bottom-up buffer selection and electrical audit - #11900

Open
naveenvenk17 wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-cts-bottom-up-buffering-11670
Open

naveenvenk17 wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
naveenvenk17:nv-cts-bottom-up-buffering-11670

Conversation

@naveenvenk17

@naveenvenk17 naveenvenk17 commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

This adds -bottom_up_buffering to clock_tree_synthesis as an opt-in step toward #11670. It keeps the H-tree topology sites, then compares buffered and wire-only alternatives using the subtree's pin and wire load instead of choosing a convenient load bucket from the LUT.

The search is bounded to 16 candidates per frontier. It preserves explicit root/tree/sink cell choices and compares against the characterized legacy tree when that reference is feasible. The timing objective is a model, not a guarantee about STA skew or setup timing.

After writing and latency balancing, a separate walk checks the actual tree's original sinks, generated subnets, max capacitance and max slew. Planning failures roll back. A post-write audit failure keeps the tree for diagnosis, so the input checkpoint must be reloaded before retrying. The current implementation requires equal-height buffer masters and placed, modeled input loads; external loads on the input net are rejected. Missing LEF buffer pins and missing STA port mappings are rejected before conversion or constraint lookup.

Validation was done on a Linux GCP VM, with independent review of the implementation and regressions:

  • All 96 CTS, estimation and repository-check targets passed.
  • All 61 C++ cases passed three shuffled runs, for 183 executions. The unchanged search kernels also passed 2,700 case executions under ASan/UBSan.
  • Tests cover actual Liberty characterization, multiple modes/scenes, clock aliases and explicit roots, shared input loads, missing geometry, dummy loads, macro/register splitting, clock-gater cloning, error rollback and the post-write audit.
  • Four matched Nangate45 runs covered GCD/AES with the switch off/on through CTS, legalization and global routing. Inputs, constraints, flow settings and original sink manifests were checked. All runs preserved sink coverage and passed electrical and placement checks. Eight default output files per design matched the pinned upstream-compatible controls byte-for-byte.
  • After the pin-mapping review fix, all four native runs were repeated on the final revision. Every output checkpoint matched the previous validated revision byte-for-byte, with the same connectivity, electrical and timing results.
  • Four new mapping cases cover missing physical input/output pins and missing input/output STA links. The missing-pin cases crash on the old code; the missing-link cases previously passed silently. All four now report the specific mapping error without changing the design.

Post-global-route results:

Design Metric Default Opt-in
GCD Clock buffers, including dummy loads 7 1
GCD Total cell area (um^2) 937.118 929.936
GCD Clock edge spread (ns) 0.001384 0.000979
GCD Setup worst slack (ns) -0.160008 -0.163851
GCD Setup TNS (ns) -7.168417 -7.264717
AES Total cell area (um^2) 20624.044 20624.044
AES Setup worst slack (ns) -0.005639 -0.005639
AES Setup TNS (ns) -0.016261 -0.016261

I kept this opt-in. GCD has a smaller clock tree but slightly worse setup timing; AES's reported area/timing values are unchanged. This is not an overall PPA win, a runtime-speedup claim, or detailed-route/signoff validation.

Review carryover: The earlier LEF/Liberty mapping finding is fixed, with separate missing-input, missing-output and unlinked-port tests. Internal register pins do have STA network ports, so their constraint lookup is retained.

Replaces #11870 to correct the contributor account. The implementation starts from tested revision 0c82210e9dea4f84a8439072cc4492920a96b253, with corrected commit authorship. A follow-up comment clarifies the physical-DBU split limit; executable code is unchanged. All 80 CTS-package and selected repository-check targets passed again on this update with test-result caching disabled.

CI note: upstream d61ef5a now records the missing registry checksum. No dependency versions changed. This CI refresh leaves the tested feature source unchanged.

Carry forward the reviewed changes from The-OpenROAD-Project#11870 under the correct contributor account.
The source tree matches 0c82210.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an experimental, bounded bottom-up buffering feature (-bottom_up_buffering) to TritonCTS. It adds new modules for bottom-up buffering and search algorithms, integrates them into the CTS flow, and implements comprehensive electrical audits and recovery mechanisms. Additionally, new unit and integration tests are added. The review feedback highlights a potential issue in BottomUpBuffering::split where a hardcoded minimum length threshold of 1 is used, which could cause incorrect failures depending on the database units.

Comment on lines +186 to +188
if (length < 1) {
throw std::runtime_error("No CTS buffer can drive a sink's capacitance");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The condition length < 1 uses a magic number 1 which may not be appropriate if the units are not in microns or if a very small distance is valid. If length is 0.5 dbu, this will throw an error, which seems incorrect. This should likely be length <= 0 or based on a defined minimum distance threshold.

Regenerated against the current upstream merge and checked for idempotence on the GCP VM. Only the missing Bison registry checksum is added; no dependency versions change.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@naveenvenk17
naveenvenk17 requested review from a team as code owners October 21, 2026 14:49
Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@naveenvenk17

Copy link
Copy Markdown
Contributor Author

The coordinates here are physical DBU, not microns or H-tree bins. The production legalizer rounds each inserted location back to integer DBU. The one-DBU cutoff stops trying to split a wire that cannot be made shorter on that grid; changing it to zero would keep inserting stages at the same legalized location. The impossible-sink and stuck-legalizer tests cover the failure paths.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
@naveenvenk17

Copy link
Copy Markdown
Contributor Author

Added a short comment next to the split guard to make the physical-DBU assumption explicit. No executable code changed. I reran the full CTS package and six repository checks on the GCP VM: all 80 targets passed, with test-result caching disabled. The comment was independently checked against the adapter's rounding behavior.

Keep the feature tree unchanged and refresh pull-request CI after the missing registry checksum was added to master.

Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>

@vvbandeira vvbandeira left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved from devops team side

@maliberty maliberty left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving for est [really it needs a true solution rather than defensive programming as 0 isn't right]

@maliberty

Copy link
Copy Markdown
Member

@luis201420 please review for cts

@arthurjolo

Copy link
Copy Markdown
Contributor

Thanks for your contribution, but a solution is already being implemented. I am looking at your PR and seeing if I can merge what you have with what I am working on.
Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants