Skip to content

Add timing paths export to Liberty #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 104 commits into
base: main
Choose a base branch
from

Conversation

magancarz
Copy link

@magancarz magancarz commented Jul 30, 2025

This PR adds timing path export to Liberty files, as well as inline reporting of such paths in report_checks. All paths types are exported: input-register, combinational, and register-output. Each path vertex has information about its instance, cell, pin, net names, transition type, arrival, slew and capacitance values.

As an example, for this cell, OpenSTA exports paths to the new paths() group inside each pin's timing(), for each rise/fall case, along with the worst slack.

module timing_cell_dff (in, clk, out);
  input in, clk;
  output out;
  wire w1, w2;

  BUF_X1 u1 (.A(in), .Z(w1));
  DFF_X1 r1 (.D(w1), .CK(clk), .Q(w2));
  BUF_X1 u2 (.A(w2), .Z(out));
endmodule
timing() {
  related_pin : "clk";
  timing_type : setup_rising;
  rise_constraint(scalar) {
      values("0.11670");
  }
  fall_constraint(scalar) {
      values("0.23045");
  }
  paths() {
    slack : 9.76955;
    fall_data_required() {
      time: 9.84328;
      vertex("r1", "DFF_X1", "r1/CK", "clk", "^", 0.00000, 0.00000, 0.82104, 0.000000);
    }
    fall_data_arrival() {
      time: 0.07373;
      vertex("", "timing_cell_dff", "in", "", "v", 0.00000, 0.00000, 0.83524, 1.000000);
      vertex("u1", "BUF_X1", "u1/A", "in", "v", 0.00000, 0.00000, 0.83524, 0.000000);
      vertex("u1", "BUF_X1", "u1/Z", "w1", "v", 0.07373, 0.01504, 1.03013, 1.000000);
      vertex("r1", "DFF_X1", "r1/D", "w1", "v", 0.07373, 0.01504, 1.03013, 0.000000);
    }
    rise_data_required() {
      time: 9.92800;
      vertex("r1", "DFF_X1", "r1/CK", "clk", "^", 0.00000, 0.00000, 0.90958, 0.000000);
    }
    rise_data_arrival() {
      time: 0.04470;
      vertex("", "timing_cell_dff", "in", "", "^", 0.00000, 0.00000, 0.93456, 1.000000);
      vertex("u1", "BUF_X1", "u1/A", "in", "^", 0.00000, 0.00000, 0.93456, 0.000000);
      vertex("u1", "BUF_X1", "u1/Z", "w1", "^", 0.04470, 0.01908, 1.10913, 1.000000);
      vertex("r1", "DFF_X1", "r1/D", "w1", "^", 0.04470, 0.01908, 1.10913, 0.000000);
    }
  }
}

Later, when the cell is used in static timing analysis with report_checks, the exported timing paths are displayed inline in the reported path.

module timing_top (in, clk, out);
  
  input in, clk;
  output out;

  timing_cell_dff tt1(.in(in), .clk(clk), .out(out));

endmodule
Startpoint: in (input port clocked by clk)
Endpoint: tt1 (rising edge-triggered flip-flop clocked by clk)
Path Group: clk
Path Type: max

  Delay    Time   Description
---------------------------------------------------------
   0.00    0.00   clock clk (rise edge)
   0.00    0.00   clock network delay (ideal)
   0.00    0.00 v input external delay
   0.00    0.00 v in (in)
   0.00    0.00 v tt1/in (timing_cell_dff)
   0.00    0.00 v tt1/u1/A (BUF_X1)
   0.07    0.07 v tt1/u1/Z (BUF_X1)
   0.00    0.07 v tt1/r1/D (DFF_X1)
           0.07   data arrival time

  10.00   10.00   clock clk (rise edge)
   0.00   10.00   clock network delay (ideal)
   0.00   10.00   clock reconvergence pessimism
          10.00 ^ tt1/clk (timing_cell_dff)
  -0.16    9.84   library setup time
           9.84   data required time
---------------------------------------------------------
           9.84   data required time
          -0.07   data arrival time
---------------------------------------------------------
           9.77   slack (MET)

Important

Adds timing path export to Liberty files and inline reporting in report_checks, supporting input-register, combinational, and register-output paths.

  • Behavior:
    • Adds timing path export to Liberty files and inline reporting in report_checks.
    • Supports input-register, combinational, and register-output path types.
    • Each path vertex includes instance, cell, pin, net names, transition type, arrival, slew, and capacitance.
  • Code Changes:
    • Adds TimingPath and TimingPathVertex structs in TimingArc.hh.
    • Updates LibertyReader.cc to parse new timing path attributes.
    • Modifies LibertyWriter.cc to write timing paths to Liberty files.
    • Enhances ReportPath class in ReportPath.cc to handle new path reporting.
  • Tests:
    • Adds tests for timing path export and reporting in timing_cell_dff, timing_cell_comb, and timing_cell_complex.

This description was created by Ellipsis for 57817e5. You can customize this summary. It will automatically update as commits are pushed.

magancarz added 30 commits July 30, 2025 11:39
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR introduces a comprehensive timing path export feature to OpenSTA that enhances Liberty library generation and timing analysis reporting. The implementation adds the ability to export detailed timing path information directly into Liberty files within a new paths() group inside timing arcs, and enables inline reporting of these paths during report_checks operations.

The core functionality works by extending the existing timing model generation infrastructure to capture and store detailed timing path information during static timing analysis. When creating timing models using the write_timing_model command, OpenSTA now extracts three types of timing paths: input-register paths (from inputs to sequential elements), combinational paths (through combinational logic), and register-output paths (from sequential elements to outputs). Each path vertex contains comprehensive information including instance names, cell types, pin names, net names, transition types, arrival times, slew rates, and capacitance values.

The implementation spans multiple components of OpenSTA. The timing arc infrastructure (TimingArc.hh, TimingArc.cc) is extended with new data structures (TimingPathVertex, TimingPath) and methods to store timing path information alongside slack values. The Liberty writer (LibertyWriter.cc) is enhanced to export this timing path data in proper Liberty syntax within paths() groups. The Liberty reader (LibertyReader.cc, LibertyReaderPvt.hh) is updated to parse the new timing path syntax. The timing model generation process (MakeTimingModel.cc, MakeTimingModelPvt.hh) is modified to extract timing paths during analysis and associate them with timing arcs. Finally, the path reporting system (ReportPath.cc, ReportPath.hh) is enhanced to detect and display inline timing paths from pre-characterized cells during standard timing reports and JSON output.

The feature is validated through comprehensive test cases covering different cell types: timing_cell_dff (sequential paths), timing_cell_comb (combinational paths), and timing_cell_complex (hierarchical multi-cell paths). The tests verify both Liberty export functionality and inline reporting in both standard and JSON formats. The implementation maintains backward compatibility while providing enhanced visibility into timing behavior within characterized cells.

Confidence score: 4/5

• This PR is generally safe to merge with proper testing, implementing a valuable new feature without breaking existing functionality
• Score reflects the complexity and breadth of changes across multiple core systems, requiring careful validation of timing analysis accuracy
• Key files needing attention: search/ReportPath.cc, search/MakeTimingModel.cc, liberty/LibertyWriter.cc, and liberty/LibertyReader.cc due to their central role in the new functionality

25 files reviewed, 12 comments

Edit Code Review Bot Settings | Greptile

}

cell ("timing_cell_dff") {
area : 6.118
Copy link

Choose a reason for hiding this comment

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

syntax: Space missing after area value - should be area : 6.118;

Suggested change
area : 6.118
area : 6.118;

Comment on lines 42 to 47
rise_constraint(scalar) {
values("-0.02757");
}
fall_constraint(scalar) {
values("-0.07072");
}
Copy link

Choose a reason for hiding this comment

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

style: Inconsistent indentation - rise_constraint and fall_constraint blocks use tabs while other timing attributes use spaces

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to 57817e5 in 1 minute and 44 seconds. Click for details.
  • Reviewed 2472 lines of code in 25 files
  • Skipped 1 files when reviewing.
  • Skipped posting 11 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. test/timing_cell_comb.lib:1
  • Draft comment:
    The combinational cell liberty file has been updated with a new 'paths()' group exporting detailed vertex information. The timing constraints and path exports are clearly structured and their values look consistent with the design intent.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. test/timing_cell_dff.lib:48
  • Draft comment:
    The dff liberty file now exports separate timing path groups for hold and setup. The exported vertex information (instance, cell, pin, net, transition, arrival, slew, capacitance, is_driver) is clearly defined.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. test/report_json1.ok:1
  • Draft comment:
    The expected JSON output for report_checks is comprehensive – it includes startpoint, endpoint, source and target paths, and numerical values in exponential notation. Ensure tolerances for floating point rounding if needed.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None
4. test/timing_cell_complex.ok:1
  • Draft comment:
    The full (text) timing report for the 'timing_cell_complex' design clearly displays all the expected stages from source clock to data arrival and required times. It correctly reports slack values with indication if timing is met.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. test/timing_cell_complex_json.ok:1
  • Draft comment:
    The JSON output for the complex design is very detailed and consistent with the text mode report. Verify that floating point formatting and vertex ordering remain stable across runs.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
6. test/regression_vars.tcl:140
  • Draft comment:
    The regression tests are well grouped and record both example and main tests. Ensure that any environment‐dependent paths (e.g. file paths in verilog_src fields) are configured consistently in your build/test system.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
7. test/write_timing_model_scalar.ok:1
  • Draft comment:
    The scalar timing model output for the 'counter' library includes input external delay and setup/hold timing details. The expected formatting appears correct. Consider documenting any tolerances for floating point outputs.
  • Reason this comment was not posted:
    Confidence changes required: 10% <= threshold 50% None
8. test/timing_cell_dff.v:1
  • Draft comment:
    The verilog source for timing_cell_dff is concise and instantiates a BUF, a DFF, and another BUF as expected. The instance names (u1, r1, u2) match with Liberty exports.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
9. test/timing_cell_complex.v:1
  • Draft comment:
    The verilog design for the complex test instantiates both timing_cell_dff and timing_cell_comb correctly. The wiring is clear and consistent with exported timing paths.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
10. search/ReportPath.cc:1063
  • Draft comment:
    There's an extra 'string' at the end of the file (line 1063) which appears to be a stray text. It should be removed.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. test/timing_cell_comb.tcl:1
  • Draft comment:
    Typo alert: The header comment on line 1 reads "# timing cel comb example". Consider updating "cel" to "cell" if this is unintended.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While this is technically a typo, it's just in a comment that has no functional impact. The comment is correct about the typo, but fixing it won't improve code quality or functionality. Our rules specifically say not to make comments that are obvious or unimportant. Comment typos are generally not worth pointing out. The typo could potentially make the file harder to find in searches, or it could indicate a broader pattern of carelessness that should be addressed. While searchability is a minor concern, this is still just a cosmetic issue in a comment. The rules clearly state not to make unimportant comments, and comment typos fall into this category. This comment should be deleted as it points out an unimportant typo in a comment that has no functional impact on the code.

Workflow ID: wflow_bOVYhYh7wd5Aewx8

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

float slack{std::numeric_limits<float>::max()};
TimingPath combinational_delay_path{};
};

Copy link

@akashlevy akashlevy Aug 4, 2025

Choose a reason for hiding this comment

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

It might also be good to record worst internal reg2reg path. This would help identify critical paths that have reg2reg paths fully inside the sub-blocks. Without this feature, we'd have to report and load the reg2reg timing paths from all sub-blocks as well.

Let's discuss more at the next meeting.

@akashlevy
Copy link

Overall good start. Highlighted a few issues. Also want to discuss including internal reg2reg paths as those can be critical at our meeting.

Would be good to get a walkthrough of what is being calculated on the fly during reporting. I skimmed that for now, would be good to discuss during our weekly meeting.

Copy link

@akashlevy akashlevy left a comment

Choose a reason for hiding this comment

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

Looking pretty great! Last thing is to enable keeping the N worst reg2reg paths.

Choose a reason for hiding this comment

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

I think these tests will need to be changed given parallaxsw#285

Choose a reason for hiding this comment

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

Shouldn't be too tough of a change though I think

Choose a reason for hiding this comment

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

Can you merge in the latest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants