odb: 3Dblox file writers #8560
odb: 3Dblox file writers #8560
Conversation
osamahammad21
left a comment
osamahammad21
left a comment
There was a problem hiding this comment.
I haven't looked at the dbxWriter yet but here is a review of the dbvWriter.
|
@rafaelmoresco Also please address:
|
rafaelmoresco
force-pushed
the
out_blox
branch
from
e3a7cb4 to
13a85f3
Compare
rafaelmoresco
force-pushed
the
out_blox
branch
from
0b1030c to
af5bae3
Compare
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
Signed-off-by: Rafael Moresco <[email protected]>
rafaelmoresco
force-pushed
the
out_blox
branch
from
af5bae3 to
a1cd3ae
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include "baseWriter.h" | ||
|
|
||
| #include <yaml-cpp/yaml.h> | ||
|
|
There was a problem hiding this comment.
warning: included header yaml.h is not used directly [misc-include-cleaner]
| { | ||
| } | ||
|
|
||
| void BaseWriter::writeHeader(YAML::Node& header_node, odb::dbDatabase* db) |
There was a problem hiding this comment.
warning: no header providing "YAML::Node" is directly included [misc-include-cleaner]
src/odb/src/3dblox/baseWriter.cpp:5:
- #include <yaml-cpp/yaml.h>
+ #include <yaml-cpp/node/node.h>
+ #include <yaml-cpp/yaml.h>| lef_writer.writeTechAndLib(lib); | ||
| } | ||
| YAML::Node list_node; | ||
| list_node.SetStyle(YAML::EmitterStyle::Flow); |
There was a problem hiding this comment.
warning: no header providing "YAML::EmitterStyle" is directly included [misc-include-cleaner]
src/odb/src/3dblox/baseWriter.cpp:5:
- #include <yaml-cpp/yaml.h>
+ #include <yaml-cpp/emitterstyle.h>
+ #include <yaml-cpp/yaml.h>| return "~"; | ||
| } | ||
|
|
||
| std::string path = ""; |
There was a problem hiding this comment.
warning: redundant string initialization [readability-redundant-string-init]
| std::string path = ""; | |
| std::string path; |
| void writeConnection(YAML::Node& connection_node, | ||
| odb::dbChipConn* conn, | ||
| odb::dbDatabase* db); | ||
| std::string buildPath(const std::vector<dbChipInst*>& path_insts, |
There was a problem hiding this comment.
warning: no header providing "std::vector" is directly included [misc-include-cleaner]
src/odb/src/3dblox/dbxWriter.h:8:
+ #include <vector>
osamahammad21
left a comment
osamahammad21
left a comment
There was a problem hiding this comment.
I don't think hierarchical chiplet instantiation is handled in this PR. for example, let's say that the top design is instantiating a hierarchical chiplet. Where is the part that writes out the dbx of that hierarchical chiplet, not the top design?
| default: | ||
| break; | ||
| } | ||
| double u = db->getDbuPerMicron(); |
There was a problem hiding this comment.
| double u = db->getDbuPerMicron(); | |
| const double u = db->getDbuPerMicron(); |
| auto chip_type = chiplet->getChipType(); | ||
| switch (chip_type) { |
There was a problem hiding this comment.
| auto chip_type = chiplet->getChipType(); | |
| switch (chip_type) { | |
| const auto chip_type = chiplet->getChipType(); | |
| switch (chip_type) { |
or
| auto chip_type = chiplet->getChipType(); | |
| switch (chip_type) { | |
| switch (chiplet->getChipType();) { |
| auto side = region->getSide(); | ||
| switch (side) { |
There was a problem hiding this comment.
| auto side = region->getSide(); | |
| switch (side) { | |
| const auto side = region->getSide(); | |
| switch (side) { |
or
| auto side = region->getSide(); | |
| switch (side) { | |
| switch (region->getSide();) { |
| c0.SetStyle(YAML::EmitterStyle::Flow); | ||
| c1.SetStyle(YAML::EmitterStyle::Flow); | ||
| c2.SetStyle(YAML::EmitterStyle::Flow); | ||
| c3.SetStyle(YAML::EmitterStyle::Flow); | ||
| c0.push_back(rect.xMin() / u); | ||
| c0.push_back(rect.yMin() / u); | ||
| c1.push_back(rect.xMax() / u); | ||
| c1.push_back(rect.yMin() / u); | ||
| c2.push_back(rect.xMax() / u); | ||
| c2.push_back(rect.yMax() / u); | ||
| c3.push_back(rect.xMin() / u); | ||
| c3.push_back(rect.yMax() / u); |
There was a problem hiding this comment.
duplicate code. better to have a writeCoordinate method
| // TODO: Identify clone instances | ||
| instance_node["is_master"] = true; |
There was a problem hiding this comment.
is_master is deprecated, we don't care about that field neither when we read nor should we when we write.
|
|
||
| private: | ||
| utl::Logger* logger_; | ||
| std::map<int, std::vector<dbChip*>> levels_; |
There was a problem hiding this comment.
Should be std::vector<std::vector<dbChip*>> instead. The index is the key/level
| if (level.chiplets.empty()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Could this happen? Should produce an error or remove if unexpected.
| if (levels.size() == 1) { | ||
| writeFile(base_filename, db); | ||
| } else { | ||
| // Write each level to a separate file | ||
| for (const auto& level : levels) { | ||
| if (level.chiplets.empty()) { | ||
| continue; | ||
| } | ||
|
|
||
| std::string level_filename | ||
| = generateLevelFilename(base_filename, level.level); | ||
| writeLevelToFile(level_filename, level.chiplets, db); | ||
|
|
||
| logger_->info(utl::ODB, | ||
| 538, | ||
| "Wrote level {} with {} chiplets to {}", | ||
| level.level, | ||
| level.chiplets.size(), | ||
| level_filename); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think there should be a different logic between multi-level and single level designs.
| for (auto chiplet : chiplets) { | ||
| // Find all chiplets that this chiplet depends on | ||
| for (auto inst : chiplet->getChipInsts()) { | ||
| auto master_chip = inst->getMasterChip(); | ||
| for (const auto& level : levels) { | ||
| for (auto level_chiplet : level.chiplets) { | ||
| if (level_chiplet == master_chip) { | ||
| dependency_levels.insert(level.level); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not necessary. The whole idea of the dag is that you know that the current level depend on the upper level. So you should include that directly.
| std::string DbvWriter::getBaseName(const std::string& filename) | ||
| { | ||
| std::filesystem::path path(filename); | ||
| return path.stem().string(); | ||
| } | ||
|
|
||
| std::string DbvWriter::getDirectory(const std::string& filename) | ||
| { | ||
| std::filesystem::path path(filename); | ||
| return path.parent_path().string(); | ||
| } |
2 participants
Draft PR for 3Dblox writers