-
Notifications
You must be signed in to change notification settings - Fork 766
drt: fix 3D pin access via generation from bottom layers #9180
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
base: master
Are you sure you want to change the base?
drt: fix 3D pin access via generation from bottom layers #9180
Conversation
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes an issue with 3D pin access generation by correctly handling vias from bottom layers. The changes in genViaEnclosedCoords ensure the correct via bounding box is used depending on the layer, and genAPEnclosedBoundary is updated to search for vias on lower layers, not just the layer above.
The changes look correct and address the problem. I have a couple of suggestions to improve code robustness and maintainability.
| if (via_def->getLayer1Num() == layer_num) { | ||
| box = via.getLayer1BBox(); | ||
| } else { | ||
| box = via.getLayer2BBox(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logic assumes that if layer_num is not via_def->getLayer1Num(), it must be via_def->getLayer2Num(). This is generally true for vias, but it's safer to explicitly check for it to make the code more robust against unexpected inputs. Adding an else if for the second layer and a fallback case for logging a warning would be a good improvement.
if (via_def->getLayer1Num() == layer_num) {
box = via.getLayer1BBox();
} else if (via_def->getLayer2Num() == layer_num) {
box = via.getLayer2BBox();
} else {
logger_->warn(DRT, 0, "genViaEnclosedCoords: layer_num {} is not part of via {}", layer_num, via_def->getName());
return;
}
src/drt/src/pa/FlexPA_acc_point.cpp
Outdated
| if (layer_num + 1 <= getDesign()->getTech()->getTopLayerNum()) { | ||
| for (auto& [tup, via] : layer_num_to_via_defs_[layer_num + 1][1]) { | ||
| genViaEnclosedCoords(coords, rect, via, layer_num, is_curr_layer_horz); | ||
| cnt++; | ||
| if (cnt >= max_num_via_trial) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| int down_via_cnt = 0; | ||
| for (int l = layer_num - 1; | ||
| l >= layer_num - 3 && l >= getDesign()->getTech()->getBottomLayerNum(); | ||
| l--) { | ||
| if (layer_num_to_via_defs_.find(l) == layer_num_to_via_defs_.end()) { | ||
| continue; | ||
| } | ||
| for (auto& [tup, via] : layer_num_to_via_defs_[l][1]) { | ||
| genViaEnclosedCoords(coords, rect, via, layer_num, is_curr_layer_horz); | ||
| down_via_cnt++; | ||
| if (down_via_cnt >= max_num_via_trial) { | ||
| break; | ||
| } | ||
| } | ||
| if (down_via_cnt >= max_num_via_trial) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for finding vias upwards and downwards is very similar, leading to code duplication. This can be refactored into a helper lambda function to improve maintainability and readability. The lambda can encapsulate the logic of iterating through via definitions for a given cut layer, generating coordinates, and checking against max_num_via_trial.
Additionally, the current implementation for finding 'up' vias uses operator[] on layer_num_to_via_defs_, which can insert an element if the key doesn't exist. Using find and at (as done for 'down' vias) is safer. The suggested refactoring addresses both points.
auto find_and_gen_vias = [&](int cut_layer, int& count) {
if (layer_num_to_via_defs_.find(cut_layer) == layer_num_to_via_defs_.end()) {
return false;
}
for (auto& [tup, via] : layer_num_to_via_defs_.at(cut_layer)[1]) {
genViaEnclosedCoords(coords, rect, via, layer_num, is_curr_layer_horz);
count++;
if (count >= max_num_via_trial) {
return true;
}
}
return false;
};
if (layer_num + 1 <= getDesign()->getTech()->getTopLayerNum()) {
find_and_gen_vias(layer_num + 1, cnt);
}
int down_via_cnt = 0;
for (int l = layer_num - 1;
l >= layer_num - 3 && l >= getDesign()->getTech()->getBottomLayerNum();
l--) {
if (find_and_gen_vias(l, down_via_cnt)) {
break;
}
}|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Jorge Ferreira <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Jorge Ferreira <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
This PR introduces enhancements to FlexPA to improve access point generation robustness, specifically addressing connectivity for Block Terminals (BTerms) in 3D/top-level contexts and ensuring compliance with LEF58_RECTONLY layer constraints. Key Changes: Smart Handling for RECTONLY Constraints: In createMultipleAccessPoints, added logic to verify if a pin shape aligns with the layer width when LEF58_RECTONLY is present. Introduced the is_bterm parameter in genAPsFromRect, genAPEnclosedBoundary, and genAPCosted to distinguish between standard cell pins and block terminals. |
osamahammad21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jferreiraOpenRoad Could you add a test case with one bterm that needs access from below?
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
|
https://drive.google.com/file/d/1bNeVX3PoVS3VcCSjwQGLioJHcPKss-5h/view?usp=sharing You need to modify the OpenRoad binary path. ../Godfort/build/gcd/job0/route.detailed/0/replay.sh |
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
Signed-off-by: Jorge Ferreira <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
fix the issue: #7138