dpl: fix power-rail detection in getMasterPwrs#10616
Conversation
Check rails with cell borders instead of using bbox centers. Previous code would incorrectly collapsed multi-box pins to the cell middle, instead of borders, mislabeling double-height cells. Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the getMasterPwrs function in src/dpl/src/infrastructure/network.cxx to improve power and ground rail detection. Instead of using the bounding box center of the MPins, it now evaluates individual routing-layer geometries to determine if they touch the cell's top or bottom boundaries. Feedback on this PR includes cleaning up a leftover comment fragment and improving the robustness of the boundary checks by ensuring the pin geometry actually intersects the cell boundaries rather than just checking one-sided inequalities.
| if (rect.yMin() <= y_cell_bot) { | ||
| (is_pwr ? bot_has_pwr : bot_has_gnd) = true; | ||
| } | ||
| if (rect.yMax() >= y_cell_top) { | ||
| (is_pwr ? top_has_pwr : top_has_gnd) = true; | ||
| } |
There was a problem hiding this comment.
To prevent potential false positives from out-of-bounds geometries (e.g., geometries completely below y_cell_bot or completely above y_cell_top), it is safer and more robust to verify that the pin geometry actually intersects or touches the respective boundary. This can be achieved by checking that the boundary coordinate lies within the [yMin, yMax] interval of the geometry.
| if (rect.yMin() <= y_cell_bot) { | |
| (is_pwr ? bot_has_pwr : bot_has_gnd) = true; | |
| } | |
| if (rect.yMax() >= y_cell_top) { | |
| (is_pwr ? top_has_pwr : top_has_gnd) = true; | |
| } | |
| if (rect.yMin() <= y_cell_bot && rect.yMax() >= y_cell_bot) { | |
| (is_pwr ? bot_has_pwr : bot_has_gnd) = true; | |
| } | |
| if (rect.yMin() <= y_cell_top && rect.yMax() >= y_cell_top) { | |
| (is_pwr ? top_has_pwr : top_has_gnd) = true; | |
| } |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
@eder-matheus @maliberty I believe we can merge this. It's no-op for all our secure-CI designs. The incorrect rail setting only affected tap cells and one TSMC double-height cell, caused by the TSMC PDK encoding wells as power/ground pins. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
eder-matheus
left a comment
eder-matheus
left a comment
There was a problem hiding this comment.
@gudeh Is this PR resolving the issue you reported in the last F&P meeting? Where he PWR/GND pins were wrongly detected in the negotiation-based DPL when running in the private design?
| // std::cout << master->getConstName() << ", height: " << master->getHeight() | ||
| // << ", return: top_pwr:" << top_pwr << " bot_pwr:" << bot_pwr << "\n"; |
There was a problem hiding this comment.
I mean to keep this. This class does not have access to a logger object.
There was a problem hiding this comment.
Do you have any suggestion on how to proceed when we want to keep a debugPrint() but we don't have access to the logger?
There was a problem hiding this comment.
Can you just add the Logger to this class? We shouldn't rely on std::cout or similar, just Logger. If you need it here, you should be able to just add it.
There was a problem hiding this comment.
I think it is useful to have access to it at Network class. I remember having to use std::cout in the past for other things too.
I included it on the latest commit.
Yes! Although, this change is actually no-op. Even with internal incorrect values the function was returning the correct value for most cases. It was incorrect for tap cells (which should not matter for DPL, since they are fixed), and for one instance of a private design. I should present further info on the next meeting. But since this is no-op we should be good to merge, and be able to cover corner cases. |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
eder-matheus
left a comment
eder-matheus
left a comment
There was a problem hiding this comment.
Looks good to me. I'll leave the final approval and merge for @osamahammad21
osamahammad21
left a comment
osamahammad21
left a comment
There was a problem hiding this comment.
@gudeh
Two concerns:
- The edge tests (
yMin <= y_cell_bot/yMax >= y_cell_top) assume the rail reaches the placement boundary. If a lib draws the rail slightly inside the boundary, the rail is missed and the edge silently becomesPower_UNK. That's probably why the old code had more tolerance. I am not sure if this a real concern or a theoretical one, but at least it is worth a warning . - Downstream,
toRailTypein NegotiationLegalizer falls back per-edge to a fixed default (UNK bottom→VSS, UNK top→VDD), ignoring the other edge. So if one rail is detected and the other is UNK, you can end up with both edges the same type (e.g. bottom=VDD + top UNK→VDD => power/power). Since rails alternate, the unknown edge should mirror the known one.
From your concern number 1, do you think we should include a small acceptance range? We should not worry about 2 because this code will be removed with the other PR #10645, where we remove the rail checking from negotiation legalizer and only use the native one from DPL. |
|
If we include a warning for |
@gudeh It depends on what the acceptance range would be. I think a reasonable value could be the layer width, but this needs to be verified against the current designs. Tho I am not sure if any of the libraries we have currently may hit that corner case. It is also worth noting that if the hypothesis is wrong, the designs may still silently pass, since DPL does not currently check the power pins against the physical PDN grid. You may notice we infer the row bottom and top power types from the cells. |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
fix incorrect l values for min and max pwr and gnd by using geometries and properly calculate the pin locations, avoid fallback to VSS for unknown power type, include skipping non routing layers Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
|
I rolled back to the previous implementation version (we should not need to worry about item 1 from @osamahammad21) with a few modifications: Now we skip non-routing layers, and we use geometry to properly calculate the internal pin positions. The previous version even had a comment asking about using geometry or box. We were also setting as VSS some masters which have undefined power pin shapes. Now they are unk. I do not observe any issue anymore with a private design which was being mislabeled. |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
3 participants
Check rails with cell borders instead of using bbox centers. Previous code would incorrectly collapsed multi-box pins to the cell middle, instead of borders, mislabeling double-height cells.
Type of Change
Impact
Private designs do not fail anymore.
Verification
./etc/Build.sh).Related Issues
This is one of multiple PRs splitting up PR #10226 to make Negotiation the default algorithm at DPL.