Repository navigation
Conversation
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
![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 introduces a new repair_pin_access feature to move standard cells to legal sites where their pin-access vias clear fixed supply geometry, integrating Opendp with TritonRoute's PinAccessService. The review feedback highlights several critical areas for improvement: potential double-restore/rollback bugs in PinAccessRepair::commit and PinAccessRepair::recover during error handling, potential null pointer dereferences in TritonRoute::isPinAccessRequired and FlexPA::updateDirtyRecoveryInsts, and a validation error in Opendp.tcl that incorrectly prevents setting -recovery_attempts to 0.
| int commit(const std::vector<Move>& moves, int remaining) | ||
| { | ||
| try { | ||
| for (const Move& move : moves) { | ||
| const odb::Point location = dp_.getDplLocation(move.cell); | ||
| move.cell->getDbInst()->setLocation(location.x(), location.y()); | ||
| } | ||
| pa->updateDirtyPinAccess(); | ||
| // AP objects may have been replaced by the native dirty-class refresh. | ||
| AccessRound fresh(supply, pa, min_access_points); | ||
| const int count = scan(fresh, true).size(); | ||
| if (count >= remaining) { | ||
| restore(moves); | ||
| return remaining; | ||
| } | ||
| return count; | ||
| } catch (...) { | ||
| try { | ||
| restore(moves); | ||
| } catch (const std::exception& error) { | ||
| dp_.logger_->warn(utl::DPL, | ||
| 513, | ||
| "Placement restored, but pin access refresh after " | ||
| "rollback failed: {}", | ||
| error.what()); | ||
| } | ||
| throw; | ||
| } | ||
| } |
There was a problem hiding this comment.
In PinAccessRepair::commit, if restore(moves) at line 716 throws an exception, it will be caught by the outer catch (...) block at line 720, which will then call restore(moves) again at line 722. This double-restore can lead to undefined behavior or nested exceptions if the first restore failed or partially succeeded. Introduce a boolean flag to track if the restore has already been executed to prevent calling it twice.
int commit(const std::vector<Move>& moves, int remaining)
{
bool restored = false;
try {
for (const Move& move : moves) {
const odb::Point location = dp_.getDplLocation(move.cell);
move.cell->getDbInst()->setLocation(location.x(), location.y());
}
pa->updateDirtyPinAccess();
// AP objects may have been replaced by the native dirty-class refresh.
AccessRound fresh(supply, pa, min_access_points);
const int count = scan(fresh, true).size();
if (count >= remaining) {
restored = true;
restore(moves);
return remaining;
}
return count;
} catch (...) {
if (!restored) {
try {
restore(moves);
} catch (const std::exception& error) {
dp_.logger_->warn(utl::DPL,
513,
"Placement restored, but pin access refresh after "
"rollback failed: {}",
error.what());
}
}
throw;
}
}| restorePlacement({move}); | ||
| pa->updatePinAccessRecovery(); | ||
| if (missing() != unresolved) { | ||
| dp_.logger_->error( | ||
| utl::DPL, | ||
| 518, | ||
| "Pin access recovery could not restore the required-pin access " | ||
| "state after a rejected trial."); | ||
| } | ||
| moves.pop_back(); |
There was a problem hiding this comment.
In PinAccessRepair::recover, if pa->updatePinAccessRecovery() at line 504 throws an exception, moves.pop_back() at line 512 is never reached. Since move was already added to moves at line 476, the exception will propagate to the outer catch (...) block at line 526, which will call restorePlacement(moves) and restore move again. Since restorePlacement({move}) was already executed at line 503, this results in a redundant double-restore. Popping move from moves before calling restorePlacement({move}) avoids this issue.
| restorePlacement({move}); | |
| pa->updatePinAccessRecovery(); | |
| if (missing() != unresolved) { | |
| dp_.logger_->error( | |
| utl::DPL, | |
| 518, | |
| "Pin access recovery could not restore the required-pin access " | |
| "state after a rejected trial."); | |
| } | |
| moves.pop_back(); | |
| moves.pop_back(); | |
| restorePlacement({move}); | |
| pa->updatePinAccessRecovery(); | |
| if (missing() != unresolved) { | |
| dp_.logger_->error( | |
| utl::DPL, | |
| 518, | |
| "Pin access recovery could not restore the required-pin access " | |
| "state after a rejected trial."); | |
| } |
| auto inst = design_->getTopBlock()->findInst(term->getInst()); | ||
| const auto index = term->getMTerm()->getIndex(); | ||
| auto inst_term = inst && index < inst->getInstTerms().size() | ||
| ? inst->getInstTerm(index) | ||
| : nullptr; |
There was a problem hiding this comment.
In TritonRoute::isPinAccessRequired, design_->getTopBlock() is called without checking if it is null. If the design is empty or not fully initialized, getTopBlock() can return nullptr, leading to a null pointer dereference. Add a null check for getTopBlock() before calling findInst.
auto top_block = design_->getTopBlock();
auto inst = top_block ? top_block->findInst(term->getInst()) : nullptr;
const auto index = term->getMTerm()->getIndex();
auto inst_term = inst && index < inst->getInstTerms().size()
? inst->getInstTerm(index)
: nullptr;| for (auto inst : dirty_insts_) { | ||
| auto old_class = unique_insts_.getUniqueClass(inst); | ||
| auto new_class = unique_insts_.computeUniqueClass(inst); | ||
| if (old_class != nullptr) { | ||
| affected.insert(old_class); | ||
| } | ||
| if (old_class != new_class) { | ||
| unique_insts_.deleteInst(inst); | ||
| unique_insts_.addInst(inst); | ||
| } | ||
| affected.insert(new_class); | ||
| } |
There was a problem hiding this comment.
In FlexPA::updateDirtyRecoveryInsts, new_class is computed and inserted into affected without checking if it is null. However, old_class is explicitly checked for null. If new_class is null, inserting it into affected will cause a null pointer dereference when iterating over affected and calling unique_class->resetSkipTerms(). Add a null check for new_class before inserting it.
for (auto inst : dirty_insts_) {
auto old_class = unique_insts_.getUniqueClass(inst);
auto new_class = unique_insts_.computeUniqueClass(inst);
if (old_class != nullptr) {
affected.insert(old_class);
}
if (old_class != new_class) {
unique_insts_.deleteInst(inst);
unique_insts_.addInst(inst);
}
if (new_class != nullptr) {
affected.insert(new_class);
}
}| set recovery_attempts 0 | ||
| if { [info exists keys(-recovery_attempts)] } { | ||
| set recovery_attempts $keys(-recovery_attempts) | ||
| sta::check_positive_integer "-recovery_attempts" $recovery_attempts | ||
| } |
There was a problem hiding this comment.
In repair_pin_access, -recovery_attempts is validated using sta::check_positive_integer, which rejects 0. However, the documentation states that recovery attempts may be zero, and the C++ implementation explicitly allows 0. Change the validation to allow non-negative integers.
set recovery_attempts 0
if { [info exists keys(-recovery_attempts)] } {
set recovery_attempts $keys(-recovery_attempts)
if { ![string is integer -strict $recovery_attempts] || $recovery_attempts < 0 } {
utl::error DPL 349 "-recovery_attempts requires a non-negative integer."
}
}
9ab5011 to
e84031d
Compare
|
@bnmfw FYI |
Add repair_pin_access after native pin access generation. Move cells whose connected pins have insufficient access or blocked escape geometry, refresh affected pin access, and restore placement when recovery fails. Keep OpenDB access-point lifetimes, special-wire net ownership and the DRT recovery service with the DPL command and its regression coverage. Signed-off-by: Philippe Sauter <phsauter@iis.ee.ethz.ch>
e84031d to
863f5cf
Compare
Summary
Adds the command
repair_pin_accessas an addition to the works started with #11120.The command is meant to be run after
pin_accessand uses PA information to slightly move a few cells with bad pin access traits in rounds. It has two distinct goals:pin_accessthinks they have valid APs but once you look at the escape routing, you can actually see that it is blocked by something else like PDN geometry.It is implemented as a separate command but could feasible be added to
detailed_placementas well (where it would only take effect if PA information is available).I also thought about adding it to
pin_accessbut decided against it as this command is currently just analytical and it is probably better to keep it that way.Regarding point 1, there are definitely improvements that could be made here like figuring out which grid offsets and orientations are good or bad for cells and directly using that to move the cell once to a better locations instead of using rounds of trials like I do now.
The downside is that this would cost a quite lengthy up-front computation of all pins in all cells against every grid offset and orientation, which is rather expensive (I have a naively parallelized implementation that does it for a library with ~1000 cell masters in about five hours).
If this is something you would like to go for (the advantage being that the data is available from the first
detailed_placement), this data would have to be saved and reloaded from a file for each technology to not make it too expensive. This would actually overall then save runtime as it would pre-solve most of pin-access.Type of Change
Impact
Adds a new command that can be run between
pin_accessanddetailed_routingto fix some routability of some badly placed cells. I would expect only a small amount of cells to move for a few designs and only for standard cell libraries with hard to access pins.As one point of data: For a ML accelerator design with more than 500k cells and a commercial standard cell library in a process with coloring, only 85 cells had to be moved to give each pin at least two valid access points.
Verification
./etc/Build.sh).Disclosure
The Readme entry is currently written by Codex (GPT-6.1 Sol). The code is majority written by Claude (Opus 5.5 and Sonnet 5.5) with reviews by me and Codex. Tests are implemented by Claude and reviewed by Codex with a final review by me to check that all add value and make sense.