Fix ODB: DEF_file in 3dbx (Issue #10077)#10096
Fix ODB: DEF_file in 3dbx (Issue #10077)#10096openroad-ci wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
![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 logic to read DEF files for chiplets and chip instances, using a boolean property to track whether the file has already been processed. The review highlights a potential for redundant DEF reads in createChiplet due to a missing check, suggests refactoring the duplicated DEF reading logic into a shared helper function, and recommends using std::vector::assign for more efficient population of the search_libs vector.
| chiplet.external.def_file.c_str(), | ||
| chip, | ||
| /*issue_callback*/ false); | ||
| odb::dbBoolProperty::create(chip, "def_file_read", true); |
There was a problem hiding this comment.
While setting the def_file_read property here is correct, the corresponding check to see if it's already set (similar to the one added in createChipInst at line 683) is missing at the beginning of the DEF reading block in this function (around line 456). This could lead to redundant DEF reads if createChiplet is called multiple times for the same master chip.
| if (!chip_inst.external.def_file.empty()) { | ||
| if (odb::dbProperty::find(chip, "def_file_read") == nullptr) { | ||
| if (chip->getBlock() != nullptr) { | ||
| odb::dbBlock::destroy(chip->getBlock()); | ||
| } | ||
| odb::defin def_reader(db_, logger_, odb::defin::DEFAULT); | ||
| std::vector<odb::dbLib*> search_libs; | ||
| for (odb::dbLib* lib : db_->getLibs()) { | ||
| search_libs.push_back(lib); | ||
| } | ||
| def_reader.readChip( | ||
| search_libs, chip_inst.external.def_file.c_str(), chip, false); | ||
| odb::dbBoolProperty::create(chip, "def_file_read", true); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for reading a DEF file for a chiplet is duplicated between createChiplet and createChipInst. This logic (checking for the property, destroying the existing block, populating search libraries, and reading the chip) should be refactored into a shared free function within a namespace to improve maintainability and ensure consistency.
References
- Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.
src/odb/src/3dblox/3dblox.cpp
Outdated
| std::vector<odb::dbLib*> search_libs; | ||
| for (odb::dbLib* lib : db_->getLibs()) { | ||
| search_libs.push_back(lib); | ||
| } |
There was a problem hiding this comment.
The population of the search_libs vector can be simplified using std::vector::assign. This is more efficient and follows the general rule for copying elements from a dbSet to a std::vector.
std::vector<odb::dbLib*> search_libs;
search_libs.assign(db_->getLibs().begin(), db_->getLibs().end());References
- To copy elements from a dbSet to a std::vector, use the vector::assign() method with iterators from the dbSet, as direct assignment is not supported.
…ard (Issue The-OpenROAD-Project#10077) Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
fix issue #10077