rsz: Buffering and sizing improvements #6564

openroad-ci · 2025-01-21T17:20:51Z

This PR combines a number of related resizer changes impacting QoR:

Improvements to gain buffering (repair_design -buffer_gain)
Pre-selection of buffers used for repair to filter out slow and delay buffers (except for hold repair)
Replacing resize to target slew with a fanout-of-4 rule
Restructuring of repair_design to do minimal resizing in response to a violation

It supersedes #6332

Signed-off-by: Martin Povišer <povik@cutebit.org>

povik · 2025-01-21T17:26:11Z

Dashboard link for when this PR is combined with appropriate flow script changes: https://dashboard.precisioninno.com/compare?sourceAType=Commit&sourceBType=Commit&sourceBName=3ac73fd1cbba34b80953390fc9412c84c3921133&sourceBID=16&sourceAName=c6905f5d5dab4c61d923ba4f164eb2335359cba9&sourceAID=5004

github-actions

clang-tidy made some suggestions

github-actions · 2025-01-21T17:29:04Z

src/rsz/src/RepairDesign.cc

+      }
+      size_cin /= nports;
+      if (size_cin > cin)
+        cin = size_cin;


warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change

cin = size_cin;

if (size_cin > cin) {

cin = size_cin;

}

github-actions · 2025-01-21T17:29:04Z

src/rsz/src/Resizer.cc

+  float delay_penalty = max_drive_resist * extra_input_cap;
+
+  float wlimit = maxLoad(network_->cell(worse)),
+        blimit = maxLoad(network_->cell(better));


warning: Value stored to 'blimit' during its initialization is never read [clang-analyzer-deadcode.DeadStores]

blimit = maxLoad(network_->cell(better)); ^

Additional context

src/rsz/src/Resizer.cc:539: Value stored to 'blimit' during its initialization is never read

blimit = maxLoad(network_->cell(better)); ^

github-actions · 2025-01-21T17:29:05Z

src/rsz/src/Resizer.cc

+  float delay_penalty = max_drive_resist * extra_input_cap;
+
+  float wlimit = maxLoad(network_->cell(worse)),
+        blimit = maxLoad(network_->cell(better));


warning: unused variable 'blimit' [clang-diagnostic-unused-variable]

blimit = maxLoad(network_->cell(better)); ^

github-actions · 2025-01-21T17:29:05Z

src/rsz/src/Resizer.cc

+  // TODO: factor out
+  for (TimingArcSet* arc_set : worse->timingArcSets()) {
+    TimingRole* role = arc_set->role();
+    if (role->combinational() && arc_set->from() == win


warning: static member accessed through instance [readability-static-accessed-through-instance]

Suggested change

if (role->combinational() && arc_set->from() == win

if (sta::TimingRole::combinational() && arc_set->from() == win

github-actions · 2025-01-21T17:29:05Z

src/rsz/src/Resizer.cc

+
+  for (TimingArcSet* arc_set : better->timingArcSets()) {
+    TimingRole* role = arc_set->role();
+    if (role->combinational() && arc_set->from() == bin


warning: static member accessed through instance [readability-static-accessed-through-instance]

Suggested change

if (role->combinational() && arc_set->from() == bin

if (sta::TimingRole::combinational() && arc_set->from() == bin

github-actions · 2025-01-21T17:29:05Z

src/rsz/src/Resizer.cc

+  LibertyCellSeq sizes_by_inp_cap;
+  sizes_by_inp_cap = buffer_cells_;
+  sort(sizes_by_inp_cap,
+       [this](const LibertyCell* buffer1, const LibertyCell* buffer2) {


warning: lambda capture 'this' is not used [clang-diagnostic-unused-lambda-capture]

Suggested change

[this](const LibertyCell* buffer1, const LibertyCell* buffer2) {

[](const LibertyCell* buffer1, const LibertyCell* buffer2) {

github-actions · 2025-01-21T17:29:05Z

src/rsz/src/Resizer.cc

+  while (port_iter.hasNext()) {
+    const LibertyPort* port = port_iter.next();
+    if (port->direction() == sta::PortDirection::input()) {
+      cin += port->capacitance();


warning: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage [clang-analyzer-core.uninitialized.Assign]

cin += port->capacitance(); ^

Additional context

src/rsz/src/Resizer.cc:1483: Assuming 'inst' is non-null

LibertyCell* cell = inst ? network_->libertyCell(inst) : nullptr; ^

src/rsz/src/Resizer.cc:1483: '?' condition is true

LibertyCell* cell = inst ? network_->libertyCell(inst) : nullptr; ^

src/rsz/src/Resizer.cc:1484: Assuming the condition is true

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1484: Left side of '&&' is true

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1484: 'inst' is non-null

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1484: Left side of '&&' is true

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1484: Left side of '&&' is true

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1484: Assuming 'cell' is non-null

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1484: Left side of '&&' is true

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1484: Taking true branch

if (!network_->isTopLevelPort(drvr_pin) && inst && !dontTouch(inst) && cell ^

src/rsz/src/Resizer.cc:1486: 'cin' declared without an initial value

float cin, load_cap; ^

src/rsz/src/Resizer.cc:1491: Assuming the condition is true

if (load_cap > 0.0 && getCin(cell, cin)) { ^

src/rsz/src/Resizer.cc:1491: Left side of '&&' is true

if (load_cap > 0.0 && getCin(cell, cin)) { ^

src/rsz/src/Resizer.cc:1491: Passing value via 2nd parameter 'cin'

if (load_cap > 0.0 && getCin(cell, cin)) { ^

src/rsz/src/Resizer.cc:1491: Calling 'Resizer::getCin'

if (load_cap > 0.0 && getCin(cell, cin)) { ^

src/rsz/src/Resizer.cc:1465: Loop condition is true. Entering loop body

while (port_iter.hasNext()) { ^

src/rsz/src/Resizer.cc:1467: Assuming the condition is true

if (port->direction() == sta::PortDirection::input()) { ^

src/rsz/src/Resizer.cc:1467: Taking true branch

if (port->direction() == sta::PortDirection::input()) { ^

src/rsz/src/Resizer.cc:1468: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage

cin += port->capacitance(); ^

github-actions · 2025-01-21T17:29:05Z

src/rsz/src/Resizer.cc

+    // Includes net parasitic capacitance.
+    load_cap = graph_delay_calc_->loadCap(drvr_pin, tgt_slew_dcalc_ap_);
+    if (load_cap > 0.0 && getCin(cell, cin)) {
+      bool is_buf_inv = cell->isBuffer() || cell->isInverter();


warning: Value stored to 'is_buf_inv' during its initialization is never read [clang-analyzer-deadcode.DeadStores]

bool is_buf_inv = cell->isBuffer() || cell->isInverter(); ^

Additional context

src/rsz/src/Resizer.cc:1492: Value stored to 'is_buf_inv' during its initialization is never read

bool is_buf_inv = cell->isBuffer() || cell->isInverter(); ^

github-actions · 2025-01-21T17:29:05Z

src/rsz/src/Resizer.cc

+    // Includes net parasitic capacitance.
+    load_cap = graph_delay_calc_->loadCap(drvr_pin, tgt_slew_dcalc_ap_);
+    if (load_cap > 0.0 && getCin(cell, cin)) {
+      bool is_buf_inv = cell->isBuffer() || cell->isInverter();


warning: unused variable 'is_buf_inv' [clang-diagnostic-unused-variable]

bool is_buf_inv = cell->isBuffer() || cell->isInverter(); ^

maliberty · 2025-01-21T20:08:43Z

src/rsz/test/buffer_varying_lengths.ok

+Differences found at line 95.
+    - i1-179 BUF_X16 + PLACED ( 0 3580000 ) N ;
+    - i1-179 BUF_X8 + PLACED ( 0 3580000 ) N ;


There should be no diffs in the ok file

Ah, missed that, will fix here and in other places if there are any

maliberty · 2025-01-21T20:09:55Z

src/rsz/src/Resizer.cc

+#if 0
+  if (/* blimit exists */ blimit > 0 && wlimit > blimit) {
+    return false;
+  }
+#endif


rm unused code

maliberty · 2025-01-22T05:40:15Z

09:38:53  The following tests FAILED:
09:38:53  	366 - gpl.convergence01.tcl (Failed)
09:38:53  	367 - gpl.convergence01.py (Failed)
09:38:53  	389 - gpl.simple01-td.tcl (Failed)
09:38:53  	390 - gpl.simple01-td.py (Failed)
09:38:53  	391 - gpl.simple01-td-tune.tcl (Failed)
09:38:53  	392 - gpl.simple01-td-tune.py (Failed)

needs attention

Signed-off-by: Martin Povišer <povik@cutebit.org>

…nges

Signed-off-by: Martin Povišer <povik@cutebit.org>

github-actions · 2025-01-23T13:28:00Z

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

Signed-off-by: Martin Povišer <povik@cutebit.org>

github-actions · 2025-01-23T15:00:45Z

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

Signed-off-by: Martin Povišer <povik@cutebit.org>

github-actions · 2025-01-28T17:25:25Z

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

precisionmoon · 2025-01-29T00:57:55Z

Updated dashboard results: https://dashboard.precisioninno.com/compare?sourceAType=Commit&sourceBType=Commit&sourceName=23eb6ca160e7d660d18527060f4f4d82bdda161a&sourceAName=09ec72a7354c364b458c5c1b63bbaaad9bbd08c3&sourceAID=5061&sourceBName=23eb6ca160e7d660d18527060f4f4d82bdda161a&sourceBID=20

Overall timing QoR looks quite good. Average setup TNS improved by 9.4% and setup TNS by 6.5%. I looked at 5 worst degradations. The QoR degradation starts at GR or DR. Details are below.

https://docs.google.com/spreadsheets/d/1IhTu-hxiFy9czpuv0bu-kM2QFB11Hj0M/edit?usp=drive_link&ouid=104723144681287518905&rtpof=true&sd=true

maliberty · 2025-02-13T21:11:53Z

Please resolve the merge conflicts so we can get this in.

Signed-off-by: Martin Povišer <povik@cutebit.org>

povik · 2025-02-24T22:12:40Z

I've resolved the conflicts for now (I think there's more to come from #6728)

src/rsz/src/RepairDesign.cc

maliberty · 2025-02-24T22:58:42Z

src/gpl/test/simple01-td-tune.ok

-    final |    -16.3% |      29 |       0 |             0 |         0
+    final |   -114.9% |      75 |       1 |             1 |         0


Are these large negative values expected? Can they really be < 100%?

I'll investigate

@povik I found the bug here, I'll PR a fix soon

OK, so I assume there's a bug unrelated to this PR

yes, the number reported is actually 10x what it should be: #6766

Unfortunately you'll have to rebase the test output after merging

maliberty · 2025-02-24T23:07:35Z

src/rsz/src/Resizer.cc

 }

 // Filter equivalent cells based on the following liberty attributes:
 // - Footprint (Optional - Honored if enforced by user): Cells with the
 //   same footprint have the same layout boundary.
 // - User Function Class (Optional - Honored if found): Cells with the
 //   same user_function_class are electrically compatible.
+
+bool Resizer::areCellsSwappable(LibertyCell* existing, LibertyCell* replacement)


@precisionmoon has added area and leakage optional limits to Resizer::getSwappableCells. Should they apply here too? If so it would be better to share the logic.

Actually I see you are sharing the code but the limits seem to have disappeared in the refactoring

maliberty · 2025-02-24T23:09:18Z

src/rsz/src/Resizer.cc

+static void populateBufferCapTestPoints(LibertyCell* cell,
+                                        LibertyPort* in,
+                                        LibertyPort* out,
+                                        std::vector<float>& points)


Any reason not to use a map here rather than a vector + sort + unique?

Well map is too much (we have nothing interesting to put in as values), but std::set<float> might be fine. At first I wasn't sure whether comparison of floats is robust enough, but Internet says it's OK to use as a key

Signed-off-by: Martin Povišer <povik@cutebit.org>

maliberty · 2025-02-27T22:30:21Z

src/rsz/src/Resizer.cc

+  for (auto p : test_points) {
+    if ((wlimit == 0 || p <= wlimit) &&
+        /* ignore high C_load/C_in region */ p
+            < std::min(bin->capacitance(), win->capacitance()) * 20.0f) {


It's an arbitrary constant: we size cells for C_load/C_in <= 4 and buffers for C_load/C_in <= 9, and the value of 20 was chosen as something which is far enough from that. Some of these constants should probably be, at some point, replaced with a set_opt_config parameter

Does it make sense to make this a constexpr variable so we can better document the purpose?

Sure, at this point it's clear there will be other changes from review so let me do that too

Signed-off-by: Martin Povišer <povik@cutebit.org>

maliberty · 2025-02-27T22:30:30Z

src/rsz/src/Resizer.cc

+
+bool Resizer::bufferSizeOutmatched(LibertyCell* worse,
+                                   LibertyCell* better,
+                                   float max_drive_resist)


src/rsz/src/Resizer.cc

maliberty · 2025-02-27T22:38:33Z

src/rsz/src/RepairDesign.cc

+        && !drvr->isConstant()) {
+      float fanout, max_fanout, fanout_slack;
+      sta_->checkFanout(drvr_pin, max_, fanout, max_fanout, fanout_slack);
+      max_fanout = 10.0f;


Why 10 and ignore the cell or design limit?

This is a value which has worked well. Can we insert a flow default of max_fanout=10 or 20 after I remove this hard-coded value?

like this: https://github.com/The-OpenROAD-Project-private/OpenROAD-flow-scripts/commit/c75a1d92b0e2c9c51000042fa5045e596fe7114e

There are many flows out there. We can add it to ORFS but if this really is the best thing they maybe it is better to keep it in OR. We can always add a flag if needed.

I've removed the override. I think we can merge this and revisit later. We don't need to enable gain-based buffering in the flow right away

Signed-off-by: Martin Povišer <povik@cutebit.org>

povik · 2025-02-28T15:58:21Z

After updates the QoR from this change has degraded: https://dashboard.precisioninno.com/compare?sourceAType=Commit&sourceBType=Commit&sourceBName=3a4a4b5c10a7f036120c4bae4be4e69c1c346062&sourceBID=21&sourceAName=a2e7a0fe4d14cb0382446a3843ee594f696f7de8

I'll need to investigate.

povik · 2025-03-04T12:39:23Z

After updates the QoR from this change has degraded: https://dashboard.precisioninno.com/compare?sourceAType=Commit&sourceBType=Commit&sourceBName=3a4a4b5c10a7f036120c4bae4be4e69c1c346062&sourceBID=21&sourceAName=a2e7a0fe4d14cb0382446a3843ee594f696f7de8

I'll need to investigate.

I've found the cause: getSwappableCells stopped returning the original size. I would've wished this came with more of a warning.

Signed-off-by: Martin Povišer <povik@cutebit.org>

povik · 2025-03-04T13:38:15Z

src/rsz/src/Resizer.cc

@@ -1418,10 +1590,6 @@ LibertyCellSeq Resizer::getSwappableCells(LibertyCell* source_cell)
      source_cell_leakage = cellLeakage(source_cell);
    }
    for (LibertyCell* equiv_cell : *equiv_cells) {
-      if (equiv_cell == source_cell) {


@precisionmoon I've put source_cell back into the swappable list. I think most users of the function expect it there.

It doesn't look like putting it back breaks anything. Let me know if there's something I need address.

Actually, we want to move away from including the source cell in the list. Why go through N+1 vs. N? We want the clients to adopt to the new behavior.

povik added 14 commits January 21, 2025 17:43

rsz: Factor out areCellsSwappable

7422734

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Decouple resize to target slew from violation repair

7868c5c

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Fix tidy warnings

ae6e4d8

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Account for margin in driver slew repair

cb30344

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Pre-select buffers for gain buffering

e18f9b9

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Use pre-selected buffers for rebuffering too

6902eda

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Use fo4 on largest gate size to decide buffering need

1256f39

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Apply gain-buffering to top inputs too

6eea2d0

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Move to 'cap ratio' sizing and fix buffering

dad1cd4

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Move early sizing up front in repair_design

7e396b4

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Size buffers using higher cap ratio

6f2afb7

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Format

d3c137c

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Update tests

287d290

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Rm commented-out target slew calls

5a18cf3

Signed-off-by: Martin Povišer <povik@cutebit.org>

openroad-ci assigned povik Jan 21, 2025

povik requested review from precisionmoon and maliberty January 21, 2025 17:21

povik mentioned this pull request Jan 21, 2025

Resize to target slew removal 1 #6332

Closed

github-actions bot reviewed Jan 21, 2025

View reviewed changes

maliberty reviewed Jan 21, 2025

View reviewed changes

povik added 4 commits January 23, 2025 13:28

rsz: Tidy

f947840

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Update tests

1b42e5e

Signed-off-by: Martin Povišer <povik@cutebit.org>

Merge remote-tracking branch 'origin/master' into rsz-gain-buffer-cha…

a36b2c3

…nges

gpl: Update tests for resizer changes

a119c47

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Add 'report_fast_buffer_sizes' debugging command

bc21f9d

Signed-off-by: Martin Povišer <povik@cutebit.org>

gpl: Update test for resizer change

02fa259

Signed-off-by: Martin Povišer <povik@cutebit.org>

povik added 8 commits February 18, 2025 15:27

Merge branch 'master' into rsz-gain-buffer-changes

7944b04

rsz, gpl: Update tests after merge

acf0c1b

Signed-off-by: Martin Povišer <povik@cutebit.org>

Merge branch 'master' into rsz-gain-buffer-changes

b1679cc

rsz, gpl: Update tests after merge

2920ae5

Signed-off-by: Martin Povišer <povik@cutebit.org>

test: Update flow limits

8f86f45

Signed-off-by: Martin Povišer <povik@cutebit.org>

Merge branch 'master' into rsz-gain-buffer-changes

94f32d1

rsz, gpl: Update tests after merge

0f9e2e1

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz, gpl: Fix "differences found" in test output

676e14f

Signed-off-by: Martin Povišer <povik@cutebit.org>

openroad-ci force-pushed the rsz-gain-buffer-changes branch from e737953 to 676e14f Compare February 24, 2025 22:10

gadfort reviewed Feb 24, 2025

View reviewed changes

src/rsz/src/RepairDesign.cc Show resolved Hide resolved

maliberty reviewed Feb 24, 2025

View reviewed changes

povik added 3 commits February 27, 2025 16:50

rsz: Fix merge lossage

37604ab

Signed-off-by: Martin Povišer <povik@cutebit.org>

Merge branch 'master' into rsz-gain-buffer-changes

9db74ed

rsz: Remove areCellsSwappable after upstream changes

3d63a75

Signed-off-by: Martin Povišer <povik@cutebit.org>

maliberty reviewed Feb 27, 2025

View reviewed changes

rsz, gpl, test: Update tests after merge

e8a39a0

Signed-off-by: Martin Povišer <povik@cutebit.org>

maliberty reviewed Feb 27, 2025

View reviewed changes

rsz: Remove hard-coded max_fanout

1de0470

Signed-off-by: Martin Povišer <povik@cutebit.org>

povik added 2 commits March 4, 2025 14:14

rsz: Incorporate review

6a3d8dd

Signed-off-by: Martin Povišer <povik@cutebit.org>

rsz: Go back to including original size in swappable list

971ae43

Signed-off-by: Martin Povišer <povik@cutebit.org>

povik reviewed Mar 4, 2025

View reviewed changes

	if (role->combinational() && arc_set->from() == win
	if (sta::TimingRole::combinational() && arc_set->from() == win

	[this](const LibertyCell* buffer1, const LibertyCell* buffer2) {
	[](const LibertyCell* buffer1, const LibertyCell* buffer2) {

		final \| -16.3% \| 29 \| 0 \| 0 \| 0
		final \| -114.9% \| 75 \| 1 \| 1 \| 0

rsz: Buffering and sizing improvements #6564

Are you sure you want to change the base?

rsz: Buffering and sizing improvements #6564

Conversation

openroad-ci commented Jan 21, 2025

povik commented Jan 21, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maliberty commented Jan 22, 2025

github-actions bot commented Jan 23, 2025

github-actions bot commented Jan 23, 2025

github-actions bot commented Jan 28, 2025

precisionmoon commented Jan 29, 2025

maliberty commented Feb 13, 2025

povik commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gadfort Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povik commented Feb 28, 2025

povik commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gadfort Feb 25, 2025 •

edited

Loading

povik commented Mar 4, 2025 •

edited

Loading