dpl: avoid improve_placement crashes on no-site macros( improve_placement core dump)#9863
Conversation
Signed-off-by: hyx <2153478753@qq.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 effectively addresses a crash in improve_placement caused by macros without a defined SITE. The approach of treating these instances as fixed obstacles is robust, and the added null-pointer checks prevent the crash. The inclusion of a regression test is excellent. My review includes a couple of suggestions to reduce code duplication, which would improve the long-term maintainability of the code.
| namespace { | ||
| bool usesRowSites(const Node* node) | ||
| { | ||
| return node->getSite() != nullptr; | ||
| } | ||
| } // namespace |
There was a problem hiding this comment.
This usesRowSites helper function is also defined in src/dpl/src/optimization/detailed_manager.cxx. To avoid code duplication, consider moving this logic into a method on the Node class itself, for example bool Node::hasSite() const in Objects.h. This would improve code reuse and maintainability.
References
- Encapsulate logic to avoid code duplication.
| auto* site = node->getSite(); | ||
| if (site == nullptr) { | ||
| return; | ||
| } | ||
| if (const auto orientation = grid->getSiteOrientation(grid_x, grid_y, site)) { | ||
| node->adjustCurrOrient(*orientation); | ||
| } |
There was a problem hiding this comment.
This logic for safely updating a node's orientation is also implemented in src/dpl/src/optimization/detailed_manager.cxx as the syncOrientToRowSite helper function. To avoid this duplication, you could move syncOrientToRowSite to a shared utility header (e.g., in src/dpl/src/util/) and call it from both locations.
References
- Encapsulate logic to avoid code duplication.
1 participant
Summary
This change fixes a crash in
improve_placementwhen the design contains placed macros whose LEF masters do not define a rowSITE.Before this fix, those instances could be collected as movable multi-height cells based on geometry alone. Later, the row-based detailed placement flow assumed that every movable cell had a valid row
SITEand unconditionally queried its row orientation, which could dereference an empty optional and terminate withbad optional access.The failure is reproducible with the related issue attachment
issue_improve_placement_core_dump.tar, which captures the crash onmempool_tile_wrapandNV_NVDLA_partition_c.The fix makes
improve_placementtreat no-SITEinstances conservatively:SITESITEmacroType of Change
Impact
This change removes a correctness bug in detailed placement and prevents
improve_placementfrom crashing on designs that contain placed macros without row sites.Behaviorally, standard row-based movable cells are unchanged. The only affected instances are placed masters without a row
SITE, which are not legal row-based detailed-placement candidates. After this change they are handled as fixed blockages instead of movable multi-height cells, so the detailed placer continues to optimize standard cells while still honoring macro occupancy.This was also validated on real failing designs. After the fix:
detailed_explanation.md
mempool_tile_wrapcompletes successfully with exit code0NV_NVDLA_partition_ccompletes successfully with exit code0Verification
./etc/Build.sh).Related Issues
#9862
There is a dropbox link in the issue that provides the files to reporduce and test the bug and fixed version.