Skip to content

dbSta: add -bin_size flag to report_timing_histogram#9727

Open
Pavankumar07s wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Pavankumar07s:feature/dbSta-add-binsize-flag
Open

dbSta: add -bin_size flag to report_timing_histogram#9727
Pavankumar07s wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Pavankumar07s:feature/dbSta-add-binsize-flag

Conversation

@Pavankumar07s
Copy link

@Pavankumar07s Pavankumar07s commented Mar 11, 2026 •
edited
Loading

dbSta: add -bin_size flag to report_timing_histogram

Closes #9445

Description

Adds a -bin_size option to the report_timing_histogram command, allowing users to specify a fixed bin width (in nanoseconds) instead of a fixed number of bins.

Motivation

The existing -num_bins flag distributes slacks into equal-count bins, but users analyzing timing closure often need uniform, human-readable bucket widths (e.g., 0.1 ns, 0.05 ns) for consistent comparison across runs and design revisions.

Changes

  • src/dbSta/include/db_sta/dbSta.hh — Added bin_size parameter to reportTimingHistogram()
  • src/dbSta/src/dbSta.cc — Implemented fixed-bin algorithm with snap-to-grid alignment using floor/ceil rounding
  • src/dbSta/src/dbSta.i — Updated SWIG bridge to pass bin_size from TCL to C++
  • src/dbSta/src/dbSta.tcl — Added -bin_size flag parsing with mutual exclusivity check against -num_bins
  • src/README.md — Documented the new option
  • src/dbSta/test/report_timing_histogram.tcl — Added regression tests for -bin_size with -setup and -hold
  • src/dbSta/test/report_timing_histogram.ok — Updated expected output for new test cases

Usage

report_timing_histogram -bin_size 0.1 -setup
report_timing_histogram -bin_size 0.05 -hold

Note: -bin_size and -num_bins are mutually exclusive. Specifying both produces an error.

Example Output

[9.400, 9.500): ************************* (1)
[9.500, 9.600):  (0)
[9.600, 9.700):  (0)
[9.700, 9.800): ************************* (1)
[9.800, 9.900]: ************************************************** (2)

Bins snap to clean multiples of bin_size, ensuring consistent axis labels across design revisions.

Test Results

//src/dbSta/test:report_timing_histogram-tcl_test    PASSED

All regression tests pass, including the new -bin_size test cases.
dbsta

@Pavankumar07s Pavankumar07s force-pushed the feature/dbSta-add-binsize-flag branch from aabd075 to 0663635 Compare March 11, 2026 17:28
gemini-code-assist[bot]
gemini-code-assist bot reviewed Mar 11, 2026
View reviewed changes
Copy link
Contributor

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

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 a -bin_size option to the report_timing_histogram command, allowing users to specify a fixed bin width for the histogram. The changes include modifications to the command-line interface, C++ implementation, SWIG bridge, documentation, and test cases. The code introduces a new algorithm for generating histograms with fixed bin sizes and ensures mutual exclusivity between the -bin_size and -num_bins options.

@Pavankumar07s
Copy link
Author

Pavankumar07s commented Mar 12, 2026

Hello @maliberty , should i consider gemini review?

github-actions[bot]
github-actions bot reviewed Mar 12, 2026
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

maliberty
maliberty reviewed Mar 12, 2026
View reviewed changes
src/dbSta/src/dbSta.cc Outdated
Comment on lines +715 to +722
if (num_bins <= 0) {
logger_->warn(utl::STA, 70, "The number of bins must be positive.");
return;
}
if (bin_size < 0.0) {
logger_->warn(utl::STA, 71, "bin_size must be non-negative.");
return;
}
Copy link
Member

@maliberty maliberty Mar 12, 2026

Choose a reason for hiding this comment

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

Use Validator from utl

Copy link
Author

@Pavankumar07s Pavankumar07s Mar 12, 2026

Choose a reason for hiding this comment

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

Thankyou @maliberty , i will replace the manual warn+return pattern with utl::Validator.Looking at other usages in the codebase for the correct pattern.

src/dbSta/src/dbSta.cc Outdated
hist_max = std::ceil(max_slack / bin_size) * bin_size;
actual_bin_width = bin_size;
actual_num_bins
= static_cast<int>(std::round((hist_max - hist_min) / bin_size));
Copy link
Member

@maliberty maliberty Mar 12, 2026

Choose a reason for hiding this comment

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

Should this be ceil?

Copy link
Author

@Pavankumar07s Pavankumar07s Mar 12, 2026

Choose a reason for hiding this comment

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

Yes, you are right Mr @maliberty . Since hist_min and hist_max are already snapped to multiples of bin_size via floor/ceil, the division should be exact but may result in a floating-point value like 4.9999. std::ceil is the safe choice. I will fix.

src/dbSta/src/dbSta.cc Outdated

const char* bracket_end = (i == actual_num_bins - 1) ? "]" : ")";

logger_->report("[{:>{}.{}f}, {:>{}.{}f}{}: {} ({})",
Copy link
Member

@maliberty maliberty Mar 12, 2026

Choose a reason for hiding this comment

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

Why does this no longer user the Histogram class?

Copy link
Author

@Pavankumar07s Pavankumar07s Mar 12, 2026

Choose a reason for hiding this comment

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

ohh I have bypassed the Histogram class when implementing the fixed
bin size mode. I'll refactor to extend Histogram with a generateBinsWithFixedSize()
Thankyou @maliberty .

@maliberty
Copy link
Member

maliberty commented Mar 12, 2026

Gemini has the usual caveats as any LLM, often right but occasionally hallucinating. Read it and see if you agree rather than reflexively following its advice.

@Pavankumar07s
Copy link
Author

Pavankumar07s commented Mar 12, 2026

Alright @maliberty I will see which gemini feedback is really need to be considered.
Thankyou.

@Pavankumar07s
dbSta: add -bin_size flag to report_timing_histogram
802a47d 
Signed-off-by: Pavankumar07s <pawankumar14662693@gmail.com>
@Pavankumar07s Pavankumar07s force-pushed the feature/dbSta-add-binsize-flag branch from 0663635 to 802a47d Compare March 12, 2026 18:55
@Pavankumar07s
Copy link
Author

Pavankumar07s commented Mar 12, 2026

Hello @maliberty, could you please check again when you have time?
Thank you.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

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

@ovebryne
Copy link

ovebryne commented Mar 13, 2026

Looks good to me!

@Pavankumar07s
Copy link
Author

Pavankumar07s commented Mar 13, 2026

Thankyou @ovebryne for review.

@Pavankumar07s Pavankumar07s requested a review from maliberty March 14, 2026 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@maliberty maliberty Awaiting requested review from maliberty

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dbSta: add -bin_size flag to report_timing_histogram

3 participants

@Pavankumar07s @maliberty @ovebryne