Fix ODB: DEF_file in 3dbx (Issue #10077) by openroad-ci · Pull Request #10096 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-04-09T19:43:25Z

fix issue #10077

Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

github-actions · 2026-04-09T19:47:48Z

clang-tidy review says "All clean, LGTM! 👍"

gemini-code-assist

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.

gemini-code-assist · 2026-04-09T19:49:42Z

src/odb/src/3dblox/3dblox.cpp

                        chiplet.external.def_file.c_str(),
                        chip,
                        /*issue_callback*/ false);
+    odb::dbBoolProperty::create(chip, "def_file_read", true);


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.

gemini-code-assist · 2026-04-09T19:49:42Z

src/odb/src/3dblox/3dblox.cpp

+  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);
+    }
+  }


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.

gemini-code-assist · 2026-04-09T19:49:42Z

src/odb/src/3dblox/3dblox.cpp

+      std::vector<odb::dbLib*> search_libs;
+      for (odb::dbLib* lib : db_->getLibs()) {
+        search_libs.push_back(lib);
+      }


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>

github-actions · 2026-04-10T02:05:11Z

clang-tidy review says "All clean, LGTM! 👍"

osamahammad21

Remove writing a def file in the 3dbv for each chiplet and add a test case.

osamahammad21 · 2026-04-13T17:44:37Z

src/odb/src/3dblox/3dblox.cpp

+
+  if (!chip_inst.external.def_file.empty()) {
+    if (chip->getBlock() != nullptr) {
+      odb::dbBlock::destroy(chip->getBlock());


you probably should log an error here. There can't be 2 instances of the same chiplet with a def file each.

osamahammad21 · 2026-04-13T17:46:07Z

src/odb/src/3dblox/3dblox.cpp

+    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);


This is the same code written in createChiplet, wrap in an inline function to reduce code duplication.

osamahammad21 · 2026-04-13T17:46:46Z

src/odb/src/3dblox/3dblox.cpp

+    odb::dbBoolProperty::create(chip, "def_file_read", true);
+    if (auto* prop = odb::dbStringProperty::find(chip, "def_file_path")) {
+      prop->setValue(chiplet.external.def_file.c_str());
+    } else {
+      odb::dbStringProperty::create(
+          chip, "def_file_path", chiplet.external.def_file.c_str());
+    }


why? I don't think these properties are needed.

osamahammad21 · 2026-04-13T17:47:03Z

src/odb/src/3dblox/3dblox.cpp

+    odb::dbBoolProperty::create(chip, "def_file_read", true);
+    if (auto* prop = odb::dbStringProperty::find(chip, "def_file_path")) {
+      prop->setValue(chip_inst.external.def_file.c_str());
+    } else {
+      odb::dbStringProperty::create(
+          chip, "def_file_path", chip_inst.external.def_file.c_str());
+    }


not needed.

osamahammad21 · 2026-04-13T17:49:27Z

src/odb/src/3dblox/dbxWriter.cpp

+  auto* chip = inst->getMasterChip();
+  if (odb::dbProperty::find(chip, "def_file_read") != nullptr) {
+    YAML::Node external_node = instance_node["external"];
+    external_node["def_file"] = std::string(chip->getName()) + ".def";
+  }


This is going to end up in all instances of that chiplet having a def file defined. This is incorrect. There should only be one chipInst with a defined def_file. It doesn't matter which one, but it should be only one.

Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

github-actions · 2026-04-14T22:17:54Z

clang-tidy review says "All clean, LGTM! 👍"

jferreiraOpenRoad · 2026-04-14T22:23:45Z

All requested changes have been made.

Fix ODB: DEF_file in 3dbx (Issue The-OpenROAD-Project#10077)

62eb530

Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

openroad-ci assigned jferreiraOpenRoad Apr 9, 2026

github-actions bot added the size/S label Apr 9, 2026

jferreiraOpenRoad requested a review from osamahammad21 April 9, 2026 19:47

gemini-code-assist bot reviewed Apr 9, 2026

View reviewed changes

Fix: read and write def_file under ChipletInst to follow 3DBlox stand…

fc03a5e

…ard (Issue The-OpenROAD-Project#10077) Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

osamahammad21 requested changes Apr 13, 2026

View reviewed changes

jferreiraOpenRoad added 2 commits April 14, 2026 22:05

style: apply clang-format to 3dblox files

0b26cd1

Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

fix: use contains instead of count > 0

362e77f

Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

jferreiraOpenRoad requested a review from osamahammad21 April 14, 2026 22:23

Conversation

openroad-ci commented Apr 9, 2026

Uh oh!

github-actions bot commented Apr 9, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 10, 2026

Uh oh!

osamahammad21 left a comment

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 14, 2026

Uh oh!

jferreiraOpenRoad commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants