odb: preserve LEF box spacing modifiers - #11069
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the lefout::writeBox function to conditionally output SPACING and DESIGNRULEWIDTH attributes for a box. However, a critical compilation issue was identified because getMinSpacing() is not a member of dbBox. The reviewer provided a code suggestion to resolve this by retrieving the spacing value from the box's owner, such as dbObstruction or dbBPin.
| if (box->getMinSpacing() > 0) { | ||
| fmt::print(out, " SPACING {:.11g}", lefdist(box->getMinSpacing())); | ||
| } | ||
|
|
||
| if (box->getDesignRuleWidth() > 0) { | ||
| fmt::print(out, | ||
| " DESIGNRULEWIDTH {:.11g}", | ||
| lefdist(box->getDesignRuleWidth())); | ||
| } |
There was a problem hiding this comment.
The method getMinSpacing() is not a member of dbBox. Based on the provided headers, this will cause a compilation error. The author's note about not running a local build suggests this might have been missed.
The getMinSpacing() method exists on dbObstruction and dbBPin. You can access it through the box's owner. The suggestion below fixes this by checking the box owner type and retrieving the spacing value accordingly.
int min_spacing = 0;
const auto owner_type = box->getOwnerType();
if (owner_type == dbBoxOwner::OBSTRUCTION) {
if (auto* obs = static_cast<dbObstruction*>(box->getBoxOwner())) {
min_spacing = obs->getMinSpacing();
}
} else if (owner_type == dbBoxOwner::BPIN) {
if (auto* pin = static_cast<dbBPin*>(box->getBoxOwner())) {
min_spacing = pin->getMinSpacing();
}
}
if (min_spacing > 0) {
fmt::print(out, " SPACING {:.11g}", lefdist(min_spacing));
}
if (box->getDesignRuleWidth() > 0) {
fmt::print(out,
" DESIGNRULEWIDTH {:.11g}",
lefdist(box->getDesignRuleWidth()));
}|
Multiple issues to resolve before review. |
|
I fixed the compile issue in the review. SPACING now comes from the dbObstruction or dbBPin owner, and other box owners keep a zero spacing value. I also fixed the clang-format failure caught by CI. On the VM, git diff --check passes and the owner API declarations match the implementation. The full C++ build is still being covered by repository CI because the VM does not have enough build storage. |
|
I pushed the review fix and added the missing regression. The implementation now reads SPACING from the obstruction or pin owner, and modified obstructions are emitted as individual RECT entries so their attributes are not lost. On the VM, the full OpenROAD binary built in Ubuntu 24.04 and |
|
You are still missing DCO. You must have some unsigned commits. See the DCO link for more info. |
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
5298a5c to
9588d3c
Compare
|
I also fixed the DCO issue. The tested tree is now one signed commit, so the branch history no longer contains the unsigned commits flagged by the check. The VM regression result is unchanged: |
| int min_spacing = 0; | ||
| if (box->getOwnerType() == dbBoxOwner::OBSTRUCTION) { | ||
| auto* obstruction = static_cast<dbObstruction*>(box->getBoxOwner()); | ||
| min_spacing = obstruction->getMinSpacing(); | ||
| } else if (box->getOwnerType() == dbBoxOwner::BPIN) { | ||
| auto* pin = static_cast<dbBPin*>(box->getBoxOwner()); | ||
| min_spacing = pin->getMinSpacing(); | ||
| } |
There was a problem hiding this comment.
Why aren't you using dbBox::getMinSpacing() ? Why are you trying to take it from the parent?
This keeps LEF obstruction modifiers when writing abstract LEF.
An obstruction with MINSPACING or DESIGNRULEWIDTH cannot be folded into the polygon set without losing its per-box attributes, so those obstructions are written as individual RECT entries. Unmodified obstructions keep the existing polygon output path.
The regression builds an abstract LEF from a database obstruction and checks that the modifier is present in the emitted RECT.
Tested on the VM:
bazel test //src/odb/test:create_obstruction-tcl_test --test_output=errors