Skip to content

mpl: add option to store clustering data as dbGroups#7968

Merged
maliberty merged 7 commits intoThe-OpenROAD-Project:masterfrom
AcKoucher:mpl-keep-clustering
Oct 13, 2025
Merged

mpl: add option to store clustering data as dbGroups#7968
maliberty merged 7 commits intoThe-OpenROAD-Project:masterfrom
AcKoucher:mpl-keep-clustering

Conversation

@AcKoucher
Copy link
Copy Markdown
Contributor

For #7959.

Next step is to allow browsing groups using the Hierarchy Browse widget in the GUI.

Signed-off-by: Arthur Koucher <arthurkoucher@precisioninno.com>
@AcKoucher AcKoucher requested a review from maliberty August 6, 2025 18:39
@maliberty maliberty requested review from eder-matheus and removed request for maliberty August 6, 2025 18:40
@maliberty
Copy link
Copy Markdown
Member

@eder-matheus please review as I'm getting backed up

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 6, 2025

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

Signed-off-by: Arthur Koucher <arthurkoucher@precisioninno.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 6, 2025

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

Copy link
Copy Markdown
Member

@eder-matheus eder-matheus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, but I would like to see how creating the groups affects the flow after MPL. It's possible that we need to change some other tools and prevent them of looking at this groups created by MPL, since they are just for visualization and debugging.

Signed-off-by: Arthur Koucher <arthurkoucher@precisioninno.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 7, 2025

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

@maliberty
Copy link
Copy Markdown
Member

If necessary we could a distinguishing value to dbGroupType

@AcKoucher
Copy link
Copy Markdown
Contributor Author

I added a new value to dbGroupType and I'm working to make the placer understand that it should ignore it.

@AcKoucher
Copy link
Copy Markdown
Contributor Author

AcKoucher commented Aug 13, 2025

I updated this branch locally with master now that both #7992 and #7986 we're merged and I still get different placement results when running the flow. It looks like DPL is also affected by the new group, so apparently there's more work needed to allow the changes here.

@maliberty
Copy link
Copy Markdown
Member

Yes we support groups in dpl/dpo too

@github-actions
Copy link
Copy Markdown
Contributor

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

@AcKoucher
Copy link
Copy Markdown
Contributor Author

@maliberty @eder-matheus I ran a secure-CI enforcing the new flag and I got the same results as running without it, so this should be good to merge.

Copy link
Copy Markdown
Member

@eder-matheus eder-matheus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, but you have a failure on this unit test:
mpl.keep_clustering_data.tcl (Failed)

Signed-off-by: Arthur Koucher <arthurkoucher@precisioninno.com>
@github-actions
Copy link
Copy Markdown
Contributor

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

@eder-matheus
Copy link
Copy Markdown
Member

@AcKoucher I believe your unit test is failing in the bazel build due to a missing dependency on the input files. I had something similar in a recent PR, see how I fixed it here: https://github.com/The-OpenROAD-Project/OpenROAD/pull/8554/files#diff-85df7b235e804a7f2ad2d172187227f957651f66e912f40a0f41fc521c86ba53R332-R337

Signed-off-by: Arthur Koucher <arthurkoucher@precisioninno.com>
@AcKoucher
Copy link
Copy Markdown
Contributor Author

AcKoucher commented Oct 13, 2025

Thanks @eder-matheus.

There were two issues. The first one was the actual test result, the other was a missing bazel resource as you pointed out. I missed that there are multiple tests using the same resource and that's being addressed later in the file:

OpenROAD/src/mpl/test/BUILD

Lines 112 to 126 in 9a0e3df

[
filegroup(
name = test_name + "_resources",
srcs = [":regression_resources"] + glob(
[
test_name + ".*",
] + {
"io_constraints6": ["testcases/io_constraints6.def"],
"io_constraints7": ["testcases/io_constraints6.def"],
"io_constraints8": ["testcases/io_constraints6.def"],
}.get(test_name, []),
),
)
for test_name in ALL_TESTS
]

The last commit should handle it.

@github-actions
Copy link
Copy Markdown
Contributor

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

@AcKoucher
Copy link
Copy Markdown
Contributor Author

@maliberty The check failure looks unrelated to the changes here:

docker: Error response from daemon: failed to create task for container:
Unavailable: error reading from server: EOF: unavailable.
[...]

@maliberty maliberty disabled auto-merge October 13, 2025 20:56
@maliberty maliberty merged commit e56b51d into The-OpenROAD-Project:master Oct 13, 2025
12 of 13 checks passed
@maliberty
Copy link
Copy Markdown
Member

@mguthaus FYI

@AcKoucher AcKoucher deleted the mpl-keep-clustering branch October 13, 2025 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants