odb: impl 3dblox checker checkLogicalConnectivity

[ahmed532]

Now reading missing nets from verilog in 3dblox in void ThreeDBlox::createChiplet(const ChipletDef& chiplet)

[gemini-code-assist]

Code Review

This pull request introduces a logical connectivity checker in 3dblox and enhances the creation of nets from Verilog files. The implementation is comprehensive and includes a good set of tests for the new functionality. My review includes suggestions to improve path handling robustness by avoiding exceptions and adhering to repository guidelines regarding helper function definitions, optimize performance in an error-reporting path, and adopt modern C++ practices in the test code.

[gemini-code-assist]

The path handling here using std::filesystem::absolute can throw an exception on error. For more robust path resolution and for consistency with the new code in checker.cpp, consider using std::filesystem::weakly_canonical with an error_code to prevent exceptions and handle paths more gracefully. Note that helper logic should generally be defined as a free function in a namespace rather than as a local lambda within a function, as per repository guidelines. While a full free function might be overkill for this specific inline usage, avoid the lambda for simpler direct integration.

          std::error_code ec;
          auto path = std::filesystem::weakly_canonical(
              chiplet.external.verilog_file, ec);
          if (ec) {
            path = std::filesystem::absolute(chiplet.external.verilog_file, ec);
          }
          std::string full_path = path.string();

References

1. Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.

[gemini-code-assist]

The available_nets vector is being constructed, sorted, and truncated inside the loop for each bump with a missing net. Since the set of Verilog nets is the same for all bumps within a single master chip, this work is redundant and can be inefficient if there are many violations.

To improve performance, this list should be prepared once per master chip, before iterating through the regions and bumps. You can then use the pre-computed list in your fmt::format call on line 455.

References

1. When analyzing code within a loop, consider the entire loop structure. Operations at the beginning of the loop (e.g., journalBegin) can affect the logic within the loop body, potentially making similar calls inside the loop redundant.

[gemini-code-assist]

To follow modern C++ practices and improve safety, consider using std::unique_ptr for managing the blox_ object. This will handle memory deallocation automatically, removing the need for a manual delete in TearDown.

You would also need to update SetUp to use std::make_unique and remove the TearDown override completely:

// In SetUp()
blox_ = std::make_unique<ThreeDBlox>(&logger_, db_.get(), sta_.get());

// Remove TearDown()

Suggested change

	ThreeDBlox* blox_;
	std::unique_ptr<ThreeDBlox> blox_;

[github-actions]

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

[github-actions]

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

[osamahammad21]

I don't think you should keep track of the read verilog files. It doesn't make any sense that 2 3dbx files reference the same verilog file, unlike lef and lib which can be referenced/shared by different chiplets.

[osamahammad21]

why?

[osamahammad21]

Suggested change

	if (cell) {
	else {

[osamahammad21]

How would it happen that any dbChipNet exist before reading the verilog?

[osamahammad21]

You should use netIterator not portBitIterator. dbChipNet should essentially reference the nets while the ports should reference the bumps. look at OpenRoad::linkDesign for reference

[osamahammad21]

why are you reading the verilog at the checker?
The checker should only check for correctness while the parser at 3dblox should read the verilog and populate the data.

[github-actions]

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

[github-actions]

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