pdn: Add -stop_at_endcaps to limit stripes to endcap power pins #5457

gatecat · 2024-07-26T15:39:58Z

This follows the alternative approach described in #5349 (comment), with a new option to stop at the extent of the power pins of endcaps.

This prevents the power stripes extending further into the contents of the endcaps, which on gf130 causes a short on a ring contained within them.

Signed-off-by: gatecat <[email protected]>

github-actions · 2024-07-26T15:46:36Z

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

gadfort · 2024-07-26T15:47:31Z

@gatecat is there any reason this shouldn't be the default behavior? ie. no need for the new flag and just stop at the ends of the pins?

gatecat · 2024-07-26T17:02:21Z

I think that could also be okay, it did seem like something that could subtly change behaviour in ways that cause problems, although I think in all other known supported PDKs endcaps have power pins extending to the end, so there'd be no change there.

However, if we had it by default, we perhaps want a better solution to repair stages down the line re-extending the straps and spoiling the fix than just setting the shapes as locked.

gadfort

I think cut rows will be an issue with the current implementation.
Part of me feels like this is right approach for all followpins, so I'm still on the fence about the new flag (since this a default would eliminate the need).
As a new default here are the pitfalls I can see:

follow pins on M2 which don't actually follow anything, but the m1 strap, that could be solved by just using the row width if no m2 pins are found and intersect the strap of interest.
there are probably some edge cases where the slight difference in strap extent would be an issue, but shouldn't be a huge problem I think.
I would expect all the CI tests to have some slight changes on M1, but really nothing else.

gadfort · 2024-07-27T00:57:39Z

src/pdn/src/straps.cpp

+
+  std::unordered_map<int, odb::dbRow*> y_to_row;
+  for (auto row : rows) {
+    y_to_row[row->getBBox().yMin()] = row;


what happens with cut rows? this would pick the "last" found row for a given y but something with macros could have any number of rows for a given y.

gadfort · 2024-07-27T00:58:35Z

src/pdn/src/straps.cpp

+        || (!inst->isEndCap()
+            && inst->getMaster()->getType()
+                   != odb::dbMasterType::CORE_WELLTAP)) {


I feel like this will end up being way too specific. I would think that any non-macro/pad fixed cell would be a valid target for this

gadfort · 2024-07-27T00:59:19Z

src/pdn/src/straps.cpp

+    odb::dbRow* row = found_row->second;
+    if (box->xMin() == row->getBBox().xMin()) {
+      result[row].left = inst;
+    }
+    if (box->xMax() == row->getBBox().xMax()) {
+      result[row].right = inst;
+    }


this will break in cases with cut rows

To check I've understood the cut row issue correctly and can implement fixes, also for the above comment, when a row is cut by a macro, there will be two rows either side of it on the same y-location but with different x bounding boxes?

Importantly, what I'd like to confirm is there's never a case where you have a single dbRow with a "hole" inside of it?

I've fixed cut rows by indexing on lower-left and lower-right points instead of just on y-coordinates. This seems to work on a local test.

I haven't committed the test yet, because although it works well enough to check that the PDN stops at the right point, the endcap/corner insertion around macros isn't really playing well with the ring design of endcap cells that's causing problems here, so it would be quite a strange looking test floorplan with short circuits elsewhere.

gadfort · 2024-07-27T01:03:10Z

src/pdn/src/straps.cpp

+int FollowPins::getPinExtent(odb::dbInst* inst, odb::dbNet* net, bool right)
+{
+  // Find the lowest (right=false) or highest (right=true) x-coordinate of power
+  // pins in a cell
+  int extent = right ? inst->getBBox()->xMin() : inst->getBBox()->xMax();
+  const odb::dbTransform transform = inst->getTransform();
+
+  for (odb::dbITerm* iterm : inst->getITerms()) {
+    if (iterm->getNet() != net) {
+      continue;
+    }
+    for (odb::dbMPin* mpin : iterm->getMTerm()->getMPins()) {
+      for (odb::dbBox* box : mpin->getGeometry()) {
+        if (box->getTechLayer() != getLayer()) {
+          continue;
+        }
+        odb::Rect rect = box->getBox();
+        transform.apply(rect);
+        if (right) {
+          extent = std::max(extent, rect.xMax());
+        } else {
+          extent = std::min(extent, rect.xMin());
+        }
+      }
+    }
+  }
+
+  return extent;
+}


it seems like what you really want is for each "row" to have a new BBox that is bounded by the pin on that layer (or by the row itself if no pins are available)
You may need to check that these pins actually overlap with the follow pin you are aiming for to avoid issues with m2 followpins on powerswitches.

Do you have a good example of the m2 followpins case that I could use as a reference?

For now, I've added a check to make sure the y-position overlaps.

gadfort · 2024-07-27T01:09:18Z

src/pdn/src/straps.cpp

+    for (odb::dbMPin* mpin : iterm->getMTerm()->getMPins()) {
+      for (odb::dbBox* box : mpin->getGeometry()) {
+        if (box->getTechLayer() != getLayer()) {
+          continue;
+        }
+        odb::Rect rect = box->getBox();
+        transform.apply(rect);
+        if (right) {
+          extent = std::max(extent, rect.xMax());
+        } else {
+          extent = std::min(extent, rect.xMin());
+        }
+      }
+    }


what would happen if the where pins on the other side of the vertical pin? this seems to ignore that case all together

I'm not sure I quite understand the problem here?

Signed-off-by: gatecat <[email protected]>

github-actions · 2024-07-30T09:53:53Z

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

maliberty · 2024-10-01T23:00:11Z

This seems to have stalled. What remains to resolve here?

gatecat · 2024-10-02T10:35:27Z

This dropped off my radar, as we did have an albeit somewhat ugly but working workaround in odbpy. I think the remaining issues where whether or not this should be default (and if it was, finding a better solution to the issues in the repair stage undoing the work here than the current one of marking the stripes as locked); and a few other of the review comments.

pdn: Add -stop_at_endcaps to limit stripes to endcap power pins

21b0e4b

Signed-off-by: gatecat <[email protected]>

gatecat mentioned this pull request Jul 26, 2024

[WIP] pdn: Add option to cut followpins around cell obstructions #5349

Closed

maliberty requested a review from gadfort July 26, 2024 16:04

gadfort requested changes Jul 27, 2024

View reviewed changes

pdn: Handle split rows in stop_at_endcaps

3044454

Signed-off-by: gatecat <[email protected]>

gatecat force-pushed the pdn-cut-v2 branch from dd1a97d to 3044454 Compare July 30, 2024 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pdn: Add -stop_at_endcaps to limit stripes to endcap power pins #5457

pdn: Add -stop_at_endcaps to limit stripes to endcap power pins #5457

gatecat commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

gadfort commented Jul 26, 2024

gatecat commented Jul 26, 2024

gadfort left a comment

gadfort Jul 27, 2024

gadfort Jul 27, 2024

gatecat Jul 30, 2024

gadfort Jul 27, 2024

gatecat Jul 29, 2024

gatecat Jul 30, 2024

gadfort Jul 27, 2024

gatecat Jul 29, 2024

gatecat Jul 30, 2024

gadfort Jul 27, 2024

gatecat Jul 30, 2024

github-actions bot commented Jul 30, 2024

maliberty commented Oct 1, 2024

gatecat commented Oct 2, 2024

pdn: Add -stop_at_endcaps to limit stripes to endcap power pins #5457

Are you sure you want to change the base?

pdn: Add -stop_at_endcaps to limit stripes to endcap power pins #5457

Conversation

gatecat commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

gadfort commented Jul 26, 2024

gatecat commented Jul 26, 2024

gadfort left a 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

github-actions bot commented Jul 30, 2024

maliberty commented Oct 1, 2024

gatecat commented Oct 2, 2024