Feat/3dblox parser path assertions#9812
Feat/3dblox parser path assertions#9812openroad-ci wants to merge 4 commits intoThe-OpenROAD-Project:masterfrom
Conversation
![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 the dbChipPath object to the OpenROAD database, enabling the storage and retrieval of chip paths. It includes modifications to several files to integrate this new object into the database schema and parser, as well as new tests to verify the functionality. The code changes appear well-structured and follow existing patterns in the codebase. Additionally, specific feedback has been provided regarding naming conventions, operator implementation, and defensive null checks.
| if (path.name.size() <= prefix.size() || !path.name.starts_with(prefix) | ||
| || path.name.find_first_not_of("0123456789", prefix.size()) | ||
| != std::string::npos | ||
| || path.name[prefix.size()] == '0') { |
There was a problem hiding this comment.
The validation logic for the path assertion name allows names like Path01, which might be confusing. It would be better to disallow leading zeros to ensure a cleaner naming convention.
|| path.name[prefix.size()] == '0') {| bool _dbChipPath::operator<(const _dbChipPath& rhs) const | ||
| { | ||
| return true; |
There was a problem hiding this comment.
The < operator always returns true. This might not be the intended behavior. Consider implementing a meaningful comparison based on the object's attributes, or remove the operator if it's not needed.
bool _dbChipPath::operator<(const _dbChipPath& rhs)
{
return false; // Implement a meaningful comparison or remove this operator
}| // Reject duplicate path names within the same chip | ||
| if (_chip->chip_path_hash_.hasMember(name)) { | ||
| _dbDatabase* db = (_dbDatabase*) _chip->getOwner(); | ||
| db->getLogger()->error(utl::ODB, | ||
| 533, | ||
| "ChipPath {} already exists in chip {}", | ||
| name, | ||
| chip->getName()); | ||
| } |
There was a problem hiding this comment.
The error message uses chip->getName() without checking if chip is valid. Although unlikely, it's good practice to ensure chip is not null before dereferencing it to prevent potential crashes.
if (_chip->chip_path_hash_.hasMember(name)) {
_dbDatabase* db = (_dbDatabase*) _chip->getOwner();
db->getLogger()->error(utl::ODB,
533,
"ChipPath {} already exists in chip {}",
name,
chip ? chip->getName() : "<null>");![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <utility> |
There was a problem hiding this comment.
warning: included header cstdint is not used directly [misc-include-cleaner]
| #include <utility> | |
| #include <utility> |
| }, | ||
| { | ||
| "name": "entries_", | ||
| "type": "std::vector<std::pair<dbId<_dbChipRegionInst>, bool>>", |
There was a problem hiding this comment.
How does that work? Each entry should have the full path:
- std::vector<dbId<_dbChipInst>> (chip path)
- dbId<_dbChipRegionInst> (the region)
There was a problem hiding this comment.
This is a single path. where each region in the path can be negated. Then in the schema we have:
{
"parent":"dbChip",
"child":"dbChipPath",
"type":"1_n",
"tbl_name":"chip_path_tbl_",
"schema":"kSchemaChipPath",
"hash": true,
"flags": ["no-serial"]
}
| dbChipPath* chip_path = dbChipPath::create(chip, assertion.name.c_str()); | ||
| for (const auto& entry : assertion.entries) { | ||
| // Resolve the dotted path string to a live DB object | ||
| std::vector<dbChipInst*> path_insts; |
There was a problem hiding this comment.
this should be used as per my previous comment.
src/odb/src/3dblox/dbxParser.cpp
Outdated
| if (raw.find(".regions.") == std::string::npos) { | ||
| logError("DBX Path assertion entry '" + raw + "' in '" + path.name | ||
| + "' must contain '.regions.' (e.g. HBM_1.regions.r1)"); | ||
| continue; |
There was a problem hiding this comment.
logError stops execution. Remove the continue
| struct Entry | ||
| { | ||
| dbChipRegionInst* region; | ||
| bool negated; | ||
| }; |
There was a problem hiding this comment.
This could be used in the dbChipConn as well. It should be defined outside the dbChipPath scope.
There was a problem hiding this comment.
How will it be used? I think keeping this private is better?
src/odb/include/odb/db.h
Outdated
| struct Entry | ||
| { | ||
| dbChipRegionInst* region; | ||
| bool negated; |
There was a problem hiding this comment.
document the meaning of negated
Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>
Store db objects in paths instead of strings. Use idiomatic ODB db object size calculation. Use hashtable. Use named constants. Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>
operator< does not have `// NOLINTBEGIN(readability-simplify-boolean-expr)` Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>
document dbChipPath negated field Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>
openroad-ci
force-pushed
the
feat/3dblox-parser-pathAssertions
branch
from
fd46e54 to
6408817
Compare
4 participants
Related to: #9300