odb: Refactor 3DBlox checker into UnfoldedModel and ThreeDBloxValidator #9217

ahmed532 · 2026-01-08T17:56:16Z

This commit separates the monolithic 3DBlox checker into two distinct components to improve modularity, maintainability, and architectural clarity:

UnfoldedModel: Responsible for hierarchical design "unfolding".
- Recursively traverses the design hierarchy to build a flat, global representation of chips, regions, and bumps.
- Employs std::deque for internal storage to guarantee pointer stability for hierarchical object references.
- Maintains an efficient path-based map for fast object resolution.
ThreeDBloxValidator: Responsible for verification logic.
- Ports existing floating chip and overlapping chip detection to the new unfolded data structure.
- Provides a dedicated entry point for future physical alignment and connectivity rule implementations.
Checker Facade:
- Updated to serve as a clean API facade that orchestrates the UnfoldedModel construction and ThreeDBloxValidator execution.
- Maintains full backward compatibility with the current Tcl interface.

Technical Improvements:

Decouples data construction from rule analysis (Single Responsibility Principle).
Improved memory safety via stable containers for cross-referenced structs.
Modularized codebase into dedicated header/implementation files.

Verified by successfully passing all 85 existing 'odb' integration tests.

Related to: #9209

This commit separates the monolithic 3DBlox checker into two distinct components to improve modularity, maintainability, and architectural clarity: 1. UnfoldedModel: Responsible for hierarchical design "unfolding". - Recursively traverses the design hierarchy to build a flat, global representation of chips, regions, and bumps. - Handles coordinate transformation accumulation and Z-mirror (MZ) state tracking. - Employs std::deque for internal storage to guarantee pointer stability for hierarchical object references. - Maintains an efficient path-based map for fast object resolution. 2. ThreeDBloxValidator: Responsible for verification logic. - Ports existing floating chip and overlapping chip detection to the new unfolded data structure. - Provides a dedicated entry point for future physical alignment and connectivity rule implementations. 3. Checker Facade: - Updated to serve as a clean API facade that orchestrates the UnfoldedModel construction and ThreeDBloxValidator execution. - Maintains full backward compatibility with the current Tcl interface. Technical Improvements: - Decouples data construction from rule analysis (Single Responsibility Principle). - Optimized orientation handling by caching Z-flip state during recursion. - Improved memory safety via stable containers for cross-referenced structs. - Modularized codebase into dedicated header/implementation files. Verified by successfully passing all 85 existing 'odb' integration tests. Related to: The-OpenROAD-Project#9209 Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>

gemini-code-assist

Code Review

This pull request refactors the 3DBlox checker into two distinct components: UnfoldedModel for building a flattened representation of the design hierarchy, and ThreeDBloxValidator for performing the validation checks. This is an excellent improvement that adheres to the Single Responsibility Principle, resulting in more modular and maintainable code. The use of std::deque to ensure pointer stability for object references is a solid design choice. The existing validation logic for floating and overlapping chips has been cleanly ported to the new structure. The changes are well-executed and establish a strong foundation for future enhancements to the checker. I have one minor suggestion to further improve code clarity and robustness.

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

github-actions

clang-tidy made some suggestions

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

src/odb/src/3dblox/threeDBloxValidator.h

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

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

github-actions · 2026-01-08T20:08:15Z

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

jhkim-pii · 2026-01-12T01:59:15Z

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

+  std::string name;
+  for (auto* chip_inst : chip_inst_path) {
+    if (!name.empty()) {
+      name += "/";


"/" <--- Is the fixed delimiter ok?
In dbBlock, _dbBlock::hier_delimiter_ is configurable.
I am not sure if the "/" should be the same as _dbBlock::hier_delimiter_ or not.

I think you're right. I'm now using hier_delimiter and '/' as a default.

jhkim-pii · 2026-01-12T02:03:17Z

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

+
+void UnfoldedModel::build(dbChip* chip)
+{
+  for (auto chip_inst : chip->getChipInsts()) {


IMO, using auto for the trivial usage case hurts code readability. Changing to dbChipInst* would be more readable.

Thanks for the update.
If you agree that auto for trivial type can hurt readability, it is better to change other auto statements as well. : )

jhkim-pii · 2026-01-12T02:34:02Z

src/odb/src/3dblox/unfoldedModel.h

+{
+  dbChipConn* connection;
+  UnfoldedRegionFull* top_region;
+  UnfoldedRegionFull* bottom_region;


Although there are no specific implementation for top_region & bottom_region yet, they may point entries in std::vector<UnfoldedRegionFull> regions;.

If # of regions is not constant and changes dynamically after construction, the vector can be reallocated and top_region and bottom_region require proper management because of the pointer address change.

If this scenario can happen, using vector may not be a good choice.

I reviewed all other instances of vector and changed some of them to deque for pointer stability. Thanks.

Could not use vector in more places for pointer stability. Used the block delimiter and '/' by default. Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>

github-actions · 2026-01-12T08:27:33Z

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

jhkim-pii · 2026-01-12T08:59:02Z

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

+        delimiter = d;
+      }
+    }
+  }


Let's consider the following cases.

There are multiple chips. Should we allow different hierarchical delimiters in multiple chips?

If there should be a single hierarchical delimiter, storing it in ODB-level (not in block or chip) is better.

It depends on the way we use the path later. For this particular function, it does not matter as long as we're consistent. And, it does not affect anything outside the checker.

We do not need to enforce this.

In general you are right to question this, but I think it is outside the scope of this PR. I can create another issue for it.

I don't think you should infer the delimiter from the block as it will inevitably lead to misalignment and ambiguity. The delimiter should be /, as this is the implicitly defined delimiter in the 3DBlox standard.

osamahammad21

checker and threeDBloxValidator should be merged. The separation doesn’t buy us much and increases mental overhead for users of the API. API clarity is better achieved by keeping a single checker class with a small public interface, and moving the rest of the logic into private.

osamahammad21 · 2026-01-13T11:05:04Z

src/odb/src/3dblox/unfoldedModel.h

+  UnfoldedModel(utl::Logger* logger);
+  void build(dbChip* chip);


Suggested change

UnfoldedModel(utl::Logger* logger);

void build(dbChip* chip);

UnfoldedModel(utl::Logger* logger, dbChip* chip);

Since you already don't clear the member variables when you call build, then calling build multiple times is gonna lead to a misleading behavior.

osamahammad21 · 2026-01-13T12:43:07Z

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

+        delimiter = d;
+      }
+    }
+  }


I don't think you should infer the delimiter from the block as it will inevitably lead to misalignment and ambiguity. The delimiter should be /, as this is the implicitly defined delimiter in the 3DBlox standard.

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

github-actions · 2026-01-14T11:23:51Z

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

ahmed532 self-assigned this Jan 8, 2026

gemini-code-assist bot reviewed Jan 8, 2026

View reviewed changes

src/odb/src/3dblox/unfoldedModel.cpp Show resolved Hide resolved

github-actions bot reviewed Jan 8, 2026

View reviewed changes

odb: refactor UnfoldedChip::getName() for readability

afc6bf5

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

ahmed532 requested review from jhkim-pii, maliberty and osamahammad21 January 8, 2026 21:20

jhkim-pii reviewed Jan 12, 2026

View reviewed changes

odb: fix 3dblox checker delimiter and deque usage

f87d1cc

Could not use vector in more places for pointer stability. Used the block delimiter and '/' by default. Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>

ahmed532 requested a review from jhkim-pii January 12, 2026 08:45

jhkim-pii reviewed Jan 12, 2026

View reviewed changes

jhkim-pii approved these changes Jan 12, 2026

View reviewed changes

osamahammad21 requested changes Jan 13, 2026

View reviewed changes

odb: absorb threeDBloxValidator in Checker

96d6cac

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

osamahammad21 approved these changes Jan 14, 2026

View reviewed changes

osamahammad21 enabled auto-merge January 14, 2026 12:01

osamahammad21 merged commit e872bb8 into The-OpenROAD-Project:master Jan 14, 2026
13 checks passed

		UnfoldedModel(utl::Logger* logger);
		void build(dbChip* chip);

	UnfoldedModel(utl::Logger* logger);
	void build(dbChip* chip);
	UnfoldedModel(utl::Logger* logger, dbChip* chip);

odb: Refactor 3DBlox checker into UnfoldedModel and ThreeDBloxValidator #9217

odb: Refactor 3DBlox checker into UnfoldedModel and ThreeDBloxValidator #9217

Uh oh!

Conversation

ahmed532 commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 8, 2026

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!

jhkim-pii Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmed532 Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

github-actions bot commented Jan 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahmed532 commented Jan 8, 2026 •

edited

Loading

jhkim-pii Jan 12, 2026 •

edited

Loading

ahmed532 Jan 12, 2026 •

edited

Loading