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

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

Closes: #9709

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?

I do not think the tests are convoluted or anything. I think we should keep them.

Isn't that the main entry point for this PR? I'm uncertain what alternative you are proposing.

We do write the verilog file associated with each 3dbx when we call write_3dbx.

I see, idk if it is useful then or not.

github-actions · 2026-03-23T19:01:29Z

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

maliberty · 2026-03-23T23:02:04Z

Nothing to add, I'll leave it to @osamahammad21 to approve/merge

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

Remove standalone tcl command Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>

github-actions · 2026-03-26T20:13:21Z

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

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

openroad-ci force-pushed the feat/3dblox-write-top-verilog branch from 515557c to fa054e8 Compare March 23, 2026 18:53

ahmed532 requested a review from maliberty March 23, 2026 20:29

Ahmed R. Mohamed added 3 commits March 26, 2026 19:23

odb: impl 3dblox verilog writer

662cfc0

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

odb: fix 3dblox verilog writer PR comments

9eab18a

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

odb: fix 3dblox verilog writer PR comments

ac2e2ab

Remove standalone tcl command Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>

openroad-ci force-pushed the feat/3dblox-write-top-verilog branch from fa054e8 to ac2e2ab Compare March 26, 2026 20:06

ahmed532 requested a review from osamahammad21 March 26, 2026 20:23

osamahammad21 approved these changes Mar 30, 2026

View reviewed changes

osamahammad21 merged commit 918c14c into The-OpenROAD-Project:master Mar 30, 2026
15 checks passed

openroad-ci deleted the feat/3dblox-write-top-verilog branch March 30, 2026 02:44

		// 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!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 23, 2026

Uh oh!

maliberty commented Mar 23, 2026

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants