odb: impl 3dblox verilog writer by openroad-ci · Pull Request #9813 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-03-18T18:26:41Z

Closes: #9709

Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>

gemini-code-assist

Code Review

The pull request introduces a new Verilog writer for 3Dblox, enabling the generation of Verilog connectivity files for hierarchical chiplets. This functionality is integrated into the OpenRoad class and exposed via Tcl commands. New unit and integration tests have been added to cover the new features, including specific tests for bump path filtering. The changes are well-structured and leverage modern C++ features like std::ranges::sort. One comment has been updated to reference a rule regarding the use of named constants instead of magic numbers.

gemini-code-assist · 2026-03-18T18:29:34Z

include/ord/OpenRoad.hh


-#include <memory>
-#include <set>
+#include <iostream>


The <iostream> header is typically used for std::cin, std::cout, std::cerr, etc. If these are only used in the corresponding .cc implementation file, it's generally better to include <iostream> there instead of in the header file. This helps to minimize compilation dependencies for files that include OpenRoad.hh.

@ahmed532 iostream not needed. mistake?

This is weird! I do not remember adding it myself. I thought it was in the file from the beginning. I'll remove it.

gemini-code-assist · 2026-03-18T18:29:34Z

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

+  // Write the Verilog connectivity file for this HIER chiplet.
+  writeVerilog(current_dir_path + chip->getName() + ".v", chip);


The writeDbx method now has a side effect of writing a Verilog connectivity file. While this might be intended for hierarchical chiplets, the method name writeDbx (presumably for 'write Dbx') does not clearly indicate this additional output. Consider renaming the method to better reflect its full functionality or making the Verilog writing an explicit, optional step if it's not always required when writing a Dbx file.

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

gemini-code-assist · 2026-03-18T18:29:34Z

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

+    const std::map<
+        dbChipInst*,
+        std::vector<std::pair<std::string, std::string>>>::const_iterator it
+        = inst_connections.find(chip_inst);


The type declaration for it is quite verbose. Using auto would improve readability and conciseness without losing type safety.

auto it = inst_connections.find(chip_inst);

gemini-code-assist · 2026-03-18T18:29:34Z

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

+      for (size_t j = 0; j < conns.size(); j++) {
+        const bool is_last = (j + 1 == conns.size());
+        fmt::print(verilog_file,
+                   "    .{}({}){}\n",
+                   conns[j].first,
+                   conns[j].second,
+                   is_last ? "" : ",");


The loop structure for printing with commas is common but can be slightly refactored for better readability and conciseness. While functional, a pattern that handles the first element separately or prefixes subsequent elements with a comma can sometimes be cleaner.

gemini-code-assist · 2026-03-18T18:29:34Z

src/odb/test/cpp/BUILD

    name = "Test3DBloxParser",
    srcs = [
        "Test3DBloxParser.cpp",
+        "Test3DBloxVerilogWriter.cpp",


Adding Test3DBloxVerilogWriter.cpp to the srcs of Test3DBloxParser might lead to less modular test organization. It would be clearer to create a separate cc_test target for Test3DBloxVerilogWriter.cpp if its tests are distinct from Test3DBloxParser. This improves test discoverability and allows for more granular test execution.

gemini-code-assist · 2026-03-18T18:29:34Z

src/odb/test/cpp/CMakeLists.txt

 add_executable(TestGDSIn TestGDSIn.cpp)
 add_executable(TestChips TestChips.cpp)
-add_executable(Test3DBloxParser Test3DBloxParser.cpp)
+add_executable(Test3DBloxParser Test3DBloxParser.cpp Test3DBloxVerilogWriter.cpp)


Similar to the BUILD file, including Test3DBloxVerilogWriter.cpp in the Test3DBloxParser executable in CMakeLists.txt could be made more explicit. Consider creating a dedicated executable target for Test3DBloxVerilogWriter.cpp to improve test modularity and clarity.

Please address. It needs a separate executable IMO.

github-actions · 2026-03-18T18:36:04Z

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

osamahammad21

I think having 3 test cases just for the verilog writer is an overkill, we should pick only one. I think that write_3dblox_verilog_nets is sufficient, but you think otherwise, choose whichever you prefer.

osamahammad21 · 2026-03-18T21:52:58Z

include/ord/OpenRoad.hh


-#include <memory>
-#include <set>
+#include <iostream>


@ahmed532 iostream not needed. mistake?

osamahammad21 · 2026-03-18T22:25:56Z

src/odb/test/cpp/CMakeLists.txt

 add_executable(TestGDSIn TestGDSIn.cpp)
 add_executable(TestChips TestChips.cpp)
-add_executable(Test3DBloxParser Test3DBloxParser.cpp)
+add_executable(Test3DBloxParser Test3DBloxParser.cpp Test3DBloxVerilogWriter.cpp)


Please address. It needs a separate executable IMO.

osamahammad21 · 2026-03-18T22:35:41Z

src/OpenRoad.tcl


+sta::define_cmd_args "write_3dblox_verilog" {filename}
+
+proc write_3dblox_verilog { args } {


@maliberty @precisionmoon Do you think we need to expose this command?

odb: impl 3dblox verilog writer

515557c

Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>

openroad-ci assigned ahmed532 Mar 18, 2026

gemini-code-assist bot reviewed Mar 18, 2026

View reviewed changes

osamahammad21 requested changes Mar 18, 2026

View reviewed changes

		// Write the Verilog connectivity file for this HIER chiplet.
		writeVerilog(current_dir_path + chip->getName() + ".v", chip);


		sta::define_cmd_args "write_3dblox_verilog" {filename}

		proc write_3dblox_verilog { args } {

Conversation

openroad-ci commented Mar 18, 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 Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ahmed532 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ahmed532 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 18, 2026

Uh oh!

osamahammad21 left a comment

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants