Rebuffering in TD global placement #7485

openroad-ci · 2025-06-03T18:42:49Z

This is "do not merge" but please do review

We can merge once port buffer locking gets in: #7457

src/rsz/test/resize_slack1.ok

povik · 2025-06-03T18:48:28Z

src/rsz/src/CMakeLists.txt

@@ -21,6 +21,7 @@ add_library(rsz_lib
    RepairHold.cc
    RepairSetup.cc
    Rebuffer.cc
+    Rebuffer2.cc


There's duplication between Rebuffer.cc and Rebuffer2.cc which can get resolved later. The former is used in repair_timing and the latter is used for TD gpl.

github-actions · 2025-06-03T18:54:23Z

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

src/rsz/src/RepairDesign.cc

@@ -221,7 +221,7 @@ void RepairDesign::repairDesign(
      print_interval_ = min_print_interval_;
    }
    printProgress(print_iteration, false, false, repaired_net_count);
-    int max_length = resizer_->metersToDbu(max_wire_length);
+    int max_length = std::numeric_limits<int>::max();


QuantamHD · 2025-06-03T20:56:34Z

src/rsz/src/BufferedNet.cc

+    if (length() == 0) {
+      res = 0;
+      cap = 0;
+    } else {
+      const double dx
+          = resizer->dbuToMeters(std::abs(location_.x() - ref_->location().x()))
+            / resizer->dbuToMeters(length());
+      const double dy
+          = resizer->dbuToMeters(std::abs(location_.y() - ref_->location().y()))
+            / resizer->dbuToMeters(length());
+      res = dx * resizer->wireSignalHResistance(corner)
+            + dy * resizer->wireSignalVResistance(corner);
+      cap = dx * resizer->wireSignalHCapacitance(corner)
+            + dy * resizer->wireSignalVCapacitance(corner);
+    }


Could we include a helper that calculates the RCs of a wire or length of a wire and returns it to you? I think it would help clean up this code.

I think so, yes, but this is pre-existing code I just fixed it for the case length() == 0

QuantamHD · 2025-06-03T20:57:38Z

src/rsz/src/BufferedNet.cc

@@ -460,6 +472,120 @@ BufferedNetPtr Resizer::makeBufferedNetSteiner(const Pin* drvr_pin,
  return bnet;
 }

+static BufferedNetPtr makeBufferedNetFromTree2(


Could you include a short description about what this algorithm is doing/attempting to do?

I've added descriptions to these new bnet builders

QuantamHD · 2025-06-03T20:59:05Z

src/rsz/src/BufferedNet.cc

@@ -666,4 +792,12 @@ BufferedNetPtr Resizer::makeBufferedNetGroute(const Pin* drvr_pin,
  return nullptr;
 }

+bool BufferedNet::fits_envelope(Metrics target)


Should be camel case naming.

QuantamHD · 2025-06-03T20:59:36Z

src/rsz/src/BufferedNet.hh

@@ -141,6 +141,44 @@ class BufferedNet

  static constexpr int null_layer = -1;

+  struct Metrics


Could we give this a more specific struct name? There's many classes named Metrics

This one is BufferedNet::Metrics so I'm thinking it's fine

QuantamHD · 2025-06-03T21:00:59Z

src/rsz/src/Rebuffer.hh

+class Rebuffer : public sta::dbStaState
+{
+ public:


Does this new Rebuffer class fit into @mguthaus's refactoring of rsz? Should rebuffer be a move or use Moves?

Does this new Rebuffer class fit into @mguthaus's refactoring of rsz?

Up to this point it's orthogonal, we coordinate

Should rebuffer be a move or use Moves?

It can be encapsulated as a move for repair_timing later on so that repair_timing can also benefit from this algorithm

Signed-off-by: Martin Povišer <[email protected]>

Fix the `resizeWostSlackNets()` API to account for the fact that journal restore might delete nets. Signed-off-by: Martin Povišer <[email protected]>

Signed-off-by: Martin Povišer <[email protected]>

github-actions · 2025-06-04T13:37:54Z

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

povik · 2025-06-04T13:39:32Z

src/rsz/src/Resizer.tcl

-    } else {
-      set max_wire_length $min_delay_max_wire_length
-      set max_wire_length_fmt [format %.0f [sta::distance_sta_ui $max_wire_length]]
-      utl::info RSZ 58 "Using max wire length ${max_wire_length_fmt}um."


This disables wire length repair in repair_design unless a length is explicitly specified with -max_wire_length. The reasoning is that placement buffering finds good solutions satisfying all ERCs. These don't necessarily fit into any wire length bound so if repair_design enforces max wire length later in the flow, it potentially degrades the solution (seen to happen).

This may negatively impact users who don't use timing-driven placement. They will see different repair results unless they are specifying -max_wire_length.

src/rsz/src/UnbufferMove.hh

povik · 2025-06-04T18:42:45Z

src/gpl/src/replace.cpp

@@ -86,7 +86,7 @@ void Replace::reset()
  routabilityMaxInflationIter_ = 4;

  timingDrivenMode_ = true;
-  keepResizeBelowOverflow_ = 0.3;
+  keepResizeBelowOverflow_ = 1.0;


This change should be making all TD rounds non-virtual by default

maliberty

Start of a review

maliberty · 2025-06-05T22:43:40Z

src/gpl/test/convergence01.ok

+   Iter   |    Area   | Removed | Inserted | Remaining
+          |           | Buffers | Buffers  |
+-------------------------------------------------------
+        0 |     +0.0% |       0 |        0 |        64


Why does nothing change over the 64 iterations?

It looks like no buffers needed in this design

maliberty · 2025-06-05T22:44:54Z

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

+[WARNING GPL-0092] Mismatch in #cells between central object and all regions. NesterovBaseCommon: 1, Summing all regions: 0
+[INFO GPL-0107] Timing-driven: repair_design delta area: 3.724 um^2 (+0.77%)
+[INFO GPL-0108] Timing-driven: repair_design, gpl cells created: 1 (+0.00%)
+[WARNING GPL-0093] Buffer insertion count by rsz (0) and cells created by gpl (1) do not match.


These warnings seem concerning.

It's a false warning, we're planning to remove it

maliberty · 2025-06-05T22:47:42Z

src/rsz/src/Resizer.i

@@ -791,6 +793,14 @@ void set_debug_cmd(const char* net_name,
  resizer->setDebugGraphics(std::move(graphics));
 }

+void
+fully_rebuffer(Pin *pin)


Is this just a test stub? Does this need a user control?

Yes, for now it's meant for testing only

maliberty · 2025-06-05T22:51:08Z

src/rsz/src/Resizer.cc

+  NetSeq worst_slack_nets;
+  for (int i = 0; i < nets.size() * worst_slack_nets_percent_ / 100.0; i++) {
+    worst_slack_nets.push_back(nets[i]);


No need to copy, just resize down the nets vector to the desired size.

Missed that, done

maliberty · 2025-06-05T22:54:16Z

src/rsz/src/Rebuffer2.cc

+using BnetPtr = BufferedNetPtr;
+using BnetMetrics = BufferedNet::Metrics;
+
+// Template magic to make it easier to write algorithms descending


Looks like a copy-paste from Rebuffer.cc.

We should retire Rebuffer.cc later so I'm hoping a bit of duplication in the interim is ok

maliberty · 2025-06-05T22:57:57Z

src/rsz/src/Rebuffer2.cc

+            return 1;
+          }
+          default:
+            abort();


Use critical not abort which has no info

Done (here and elsewhere)

maliberty · 2025-06-05T23:23:38Z

src/rsz/src/Rebuffer2.cc

+  while (ptr->type() == BnetType::wire) {
+    ptr = ptr->ref();


Are we guaranteed the ptr will never be null (ie we can't reach the end with type wire)? Same in stripWiresAndBuffersOnBnet

Yes, ref() is always non-null on type wire or buffer

maliberty · 2025-06-05T23:24:57Z

src/rsz/src/Rebuffer2.cc

+  const double a = wire_res * wire_cap;
+  const double b = wire_res * in->capacitance() + r_drvr * wire_cap;
+  const double c = r_drvr * in->capacitance() - max_slew / elmore_skew_factor_;
+  const double D = b * b - 4 * a * c;


This looks very similar to RepairDesign::repairNetWire. Make a common utility?

repairNetWire's version has more terms. I'm on the edge as to whether an utility is worth it

maliberty · 2025-06-05T23:28:28Z

src/rsz/src/SteinerTree.hh

-  const Pin* drvr_pin_;
+  Point drvr_location_;


What necessitates being non-const?

maliberty · 2025-06-05T23:29:16Z

src/rsz/src/SteinerTree.cc

+    std::vector<int> x,
+        y;  // Two separate vectors of coordinates needed by flute.


Icky formatting, put the comment on the line above

Signed-off-by: Martin Povišer <[email protected]>

github-actions · 2025-06-06T09:24:49Z

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

Signed-off-by: Martin Povišer <[email protected]>

github-actions · 2025-06-06T14:29:02Z

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

povik · 2025-06-09T13:10:17Z

We're now getting a CI failure on merge I believe due to #7506 changing the log output. I'm holding off regoldening until we are closer to merge.

20:57:06  The following tests FAILED:

20:57:06  	430 - gpl.simple01-td.tcl (Failed)                      IntegrationTest tcl gpl log_compare
20:57:06  	431 - gpl.simple01-td.py (Failed)                       IntegrationTest python gpl log_compare
20:57:06  	432 - gpl.simple01-td-tune.tcl (Failed)                 IntegrationTest tcl gpl log_compare
20:57:06  	433 - gpl.simple01-td-tune.py (Failed)                  IntegrationTest python gpl log_compare

openroad-ci assigned povik Jun 3, 2025

povik requested a review from maliberty June 3, 2025 18:43

povik reviewed Jun 3, 2025

View reviewed changes

src/rsz/test/resize_slack1.ok Outdated Show resolved Hide resolved

povik reviewed Jun 3, 2025

View reviewed changes

QuantamHD reviewed Jun 3, 2025

View reviewed changes

povik added 10 commits June 4, 2025 14:16

rsz: Add new steiner tree utils

65b18dc

Signed-off-by: Martin Povišer <[email protected]>

rsz: Start rebuffer class

8112e98

Signed-off-by: Martin Povišer <[email protected]>

gpl, rsz: Fully rebuffer on td iterations

c742efd

Signed-off-by: Martin Povišer <[email protected]>

rsz: Remove stale declarations

74fa070

Signed-off-by: Martin Povišer <[email protected]>

rsz: Add nangate45 slew fixup

4fcdb9c

Signed-off-by: Martin Povišer <[email protected]>

rsz: Disable max length repair on data lines

17bea02

Signed-off-by: Martin Povišer <[email protected]>

rsz: Rm disabled code

738f9b1

Signed-off-by: Martin Povišer <[email protected]>

rsz, gpl, test: Update tests

3641932

Signed-off-by: Martin Povišer <[email protected]>

rsz: Fix resizeWorstSlackNets giving stale pointers

974e8a2

Fix the `resizeWostSlackNets()` API to account for the fact that journal restore might delete nets. Signed-off-by: Martin Povišer <[email protected]>

rsz, cts: Update tests

5fe1090

Signed-off-by: Martin Povišer <[email protected]>

openroad-ci force-pushed the rsz-fully-rebuffer-new branch from 18b19a8 to 5fe1090 Compare June 4, 2025 13:24

povik reviewed Jun 4, 2025

View reviewed changes

mguthaus reviewed Jun 4, 2025

View reviewed changes

src/rsz/src/UnbufferMove.hh Show resolved Hide resolved

povik reviewed Jun 4, 2025

View reviewed changes

maliberty changed the title ~~DO NOT MERGE: Rebuffering in TD global placement~~ Rebuffering in TD global placement Jun 5, 2025

maliberty reviewed Jun 5, 2025

View reviewed changes

rsz: Update for PR feedback

c42fe46

Signed-off-by: Martin Povišer <[email protected]>

rsz: Apply ceil to match old behavior

9cde95a

Signed-off-by: Martin Povišer <[email protected]>

		@@ -141,6 +141,44 @@ class BufferedNet

		static constexpr int null_layer = -1;

		struct Metrics

		std::vector<int> x,
		y; // Two separate vectors of coordinates needed by flute.

Rebuffering in TD global placement #7485

Are you sure you want to change the base?

Rebuffering in TD global placement #7485

Conversation

openroad-ci commented Jun 3, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 3, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maliberty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment