ODB: Initial support for alignment markers by openroad-ci · Pull Request #10287 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-04-28T15:57:28Z

Summary

To support alignment markers in odb & 3dblox-linter:

Introduce UnfoldedAlignmentMarker class to the unfolded model
Introduce set_alignment_marker tcl command
Add the necessary checks for alignment markers in 3dblox/checker.cpp
Add a test case for alignment markers

Type of Change

New feature

Verification

I have verified that the local build succeeds (./etc/Build.sh).
I have run the relevant tests and they pass.
My code follows the repository's formatting guidelines.
I have signed my commits (DCO).

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

gemini-code-assist

Code Review

This pull request introduces a new alignment marker check feature for 3D designs, including database schema updates, a new spatial index utility, and the implementation of the checker logic. I have identified a potential issue regarding the semantic change of chip dimensions in 3dblox.cpp, where using xMax/yMax instead of dx/dy may lead to incorrect values if the design does not start at the origin. Additionally, the current implementation of the alignment marker check may suffer from performance issues due to linear scanning; I recommend pre-building a spatial index per chip and grouping connections by chip pairs to optimize the check.

gemini-code-assist · 2026-04-28T16:01:45Z

+    auto list_a = collectMarkersInRect(c_a->alignment_markers, overlap);
+    auto list_b = collectMarkersInRect(c_b->alignment_markers, overlap);


The current implementation performs a linear scan of all markers for every connection via collectMarkersInRect. In a large 3D system with many connections and markers, this O(N_conn * N_markers) approach can be inefficient. Additionally, if multiple connections exist between the same pair of chips, the same markers may be checked and reported multiple times. Consider pre-building a spatial index for each chip's markers and grouping connections by chip pairs to improve performance and avoid redundant violations.

github-actions · 2026-04-28T16:08:33Z

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

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

github-actions · 2026-04-30T13:38:51Z

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

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

github-actions

clang-tidy made some suggestions

github-actions · 2026-05-05T18:28:45Z

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

…message 1. Bazel test detail (The-OpenROAD-Project#7). The actual stderr (e.g. `AssertionError: 20 != 19`) lives in a Bazel `==================== Test output for <target>:` block — usually thousands of lines *before* the FAILED summary row in the same log. Restructure the parser to: - record FAILED summary rows into a per-target dict (collapses re-runs from `--keep_going`); - buffer Test-output block lines unconditionally as we see them, so blocks that precede their FAILED row aren't dropped; - use a length-78+ `=` rule as the block end (Bazel's closing fence is 80 wide; the unittest framework's internal 70-wide separator no longer terminates capture early). PR The-OpenROAD-Project#10287 dogfood: `//src/odb/test:odb_man_tcl_check` finding now carries `AssertionError: 20 != 19 : ./src/odb: help count (20) != readme count (19)` in its detail / AI directive. 2. Per-finding log URL (The-OpenROAD-Project#11). cli._scan_check threads `check.details_url` into every emitted finding; render_markdown appends "Full log: <url>." to each item in the AI directive. 3. "Couldn't extract" message (The-OpenROAD-Project#10). When the discovery layer reports N failing checks but no parser produced a finding, the table now says "Found N failing check(s) but couldn't extract any actionable findings — see <urls>" instead of an empty screen. `discover_findings` returns `(findings, failing_check_urls)` so the renderer can show the URL list. fix_main.py + 4 unit tests updated for the new tuple shape. Tests added/updated: - test_bazel_test: 3 new cases covering block-before-summary, block-truncation, no-block-fallback, plus the existing assertion cases. - test_render_table: empty-with-failing-urls case. - test_cli, test_fix_runs_recipes: updated stubs for tuple return. 30/30 Bazel tests pass. Final 10-PR dogfood survey (the same set used to motivate P0): - All 10 PRs produce useful output. - PR The-OpenROAD-Project#10287's bazel_test_fail finding now contains the actual `AssertionError`. - PR The-OpenROAD-Project#10288's purged-build case shows a `log_unavailable` info finding alongside the 5 reviewable Gemini comments on its diff. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

github-actions · 2026-05-07T22:45:10Z

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

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

github-actions · 2026-05-09T19:32:52Z

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

maliberty · 2026-05-10T15:49:38Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1563bbcb9b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-10T15:56:39Z

+    const Rect overlap = ra->cuboid.getEnclosingRect().intersect(
+        rb->cuboid.getEnclosingRect());


Guard overlap before intersecting connection regions

checkAlignmentMarkers() calls Rect::intersect() on the two region rectangles without first checking they overlap. Rect::intersect() asserts on non-overlapping rectangles, so any invalid/non-overlapping dbChipConn between chips that have alignment markers can crash the checker in assert-enabled builds before it reports diagnostics. Add an intersects()/isValid(conn) guard before computing overlap.

Useful? React with 👍 / 👎.

maliberty · 2026-05-10T15:53:37Z


  bool isTsv() const;

+  void setAlignmentMarkerTolerance(int alignment_marker_tolerance);


Can each chip have a different tolerance? What happens when two bond and have different values?

Since there is no officially documented constraint driving this work, I tried to keep the implementation as flexible as possible.
The simplest approach would be to use one global tolerance value, but I am not sure that will always be sufficient for users. For that reason, I kept the alignment tolerance at the chip level. Currently, I only use the value from the top chiplet. If we later find that per-chip tolerance is needed, this will be easier to support than if the tolerance were stored under dbDatabase.

@gadfort opinion?

so far I have not seen any alignment tolerance that wasn't == 0.0 (perfect overlap).
But it seems like the tolerance belongs to the pair of alignment marks since you can't really have one chip with a tolerance of < 1.0 and another with >1.0 and < 2.0 (the tolerance is governed by the imaging tools used so it's the set of marks that make that a thing. The other check to consider (without having looked at this code), is that must be the a particular orientation with respect to eachother for them to be valid.

maliberty · 2026-05-10T15:56:34Z

+  }
+  foreach lib [$db getLibs] {
+    foreach master [$lib getMasters] {
+      $master setAlignmentMarker 0


If you ran this command twice with two different set of masters the second would zero out the first.

yes, that's true.

This seems unexpected

what do you propose?

maliberty · 2026-05-10T15:57:25Z

+    sta::check_positive_float "-tolerance" $tol
+  }
+  set db [ord::get_db]
+  set chip [$db getChip]


Is this assuming a single chip?

no, dbDatabase::getChip returns the top chiplet.

maliberty · 2026-05-10T15:57:45Z

+    }
+  }
+  set tol_dbu [expr { int($tol * [$db getDbuPerMicron]) }]
+  $chip setAlignmentMarkerTolerance $tol_dbu


It is expected that if -tolerance isn't given then it is set to 0?

It seems odd to override a value that could have been set previously when the option isn't given.

osamahammad21 added 6 commits April 21, 2026 15:59

odb: initial support for alignment markers

cb1f204

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

odb: alignment marker violation bbox

ce01a7d

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

refactor

9e211ad

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

Merge remote-tracking branch 'origin/master' into odb-alignment-markers

b13c02b

odb: requested clang-tidy changes

827655a

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

clang-tidy pragma keep include

bd4dd82

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

openroad-ci assigned osamahammad21 Apr 28, 2026

github-actions Bot added the size/L label Apr 28, 2026

gemini-code-assist Bot reviewed Apr 28, 2026

View reviewed changes

osamahammad21 requested a review from maliberty April 28, 2026 16:01

osamahammad21 added 2 commits April 29, 2026 15:11

Merge remote-tracking branch 'origin/master' into odb-alignment-markers

2e34aea

fix build failures

0aca322

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

maliberty requested changes Apr 30, 2026

View reviewed changes

osamahammad21 added 4 commits May 5, 2026 20:33

odb: address requested changes

4169835

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

odb: check alignment-marker orientation in 3DBlox checker

b7ca8a0

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

Merge remote-tracking branch 'origin/master' into odb-alignment-markers

cadedc0

clang-tidy requested changes

91ac452

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

github-actions Bot reviewed May 5, 2026

View reviewed changes

Comment thread src/OpenRoad.cc

Comment thread src/odb/include/odb/unfoldedModel.h

odb: document set_3dblox_alignment_markers in README.md

fd9dc1d

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

osamahammad21 requested a review from maliberty May 7, 2026 22:43

update ok

1563bbc

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>

chatgpt-codex-connector Bot reviewed May 10, 2026

View reviewed changes

maliberty requested changes May 10, 2026

View reviewed changes

		auto list_a = collectMarkersInRect(c_a->alignment_markers, overlap);
		auto list_b = collectMarkersInRect(c_b->alignment_markers, overlap);

		const Rect overlap = ra->cuboid.getEnclosingRect().intersect(
		rb->cuboid.getEnclosingRect());


		bool isTsv() const;

		void setAlignmentMarkerTolerance(int alignment_marker_tolerance);

Conversation

openroad-ci commented Apr 28, 2026

Summary

Type of Change

Verification

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

github-actions Bot commented May 9, 2026

Uh oh!

maliberty commented May 10, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 10, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants