Skip to content

pdn: Give pins stable order#8904

Closed
openroad-ci wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:pdn-fix-pads-test
Closed

pdn: Give pins stable order#8904
openroad-ci wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:pdn-fix-pads-test

Conversation

@openroad-ci
Copy link
Collaborator

The order seems to matter for pdngen output so make it stable. See the updated tests.

The order seems to matter for pdngen output so make it stable. See the
updated tests.

Signed-off-by: Martin Povišer <povik@cutebit.org>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

break;
}

std::sort(pins_.begin(), pins_.end(), [](odb::dbBox* l, odb::dbBox* r) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use a ranges version of this algorithm [modernize-use-ranges]

Suggested change
std::sort(pins_.begin(), pins_.end(), [](odb::dbBox* l, odb::dbBox* r) {
std::ranges::sort(pins_,, [](odb::dbBox* l, odb::dbBox* r) {

}

std::sort(pins_.begin(), pins_.end(), [](odb::dbBox* l, odb::dbBox* r) {
return std::make_tuple(l->xMin(), l->xMax(), l->yMin(), l->yMax())
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::make_tuple" is directly included [misc-include-cleaner]

src/pdn/src/straps.cpp:14:

- #include <utility>
+ #include <tuple>
+ #include <utility>

std::sort(pins_.begin(), pins_.end(), [](odb::dbBox* l, odb::dbBox* r) {
return std::make_tuple(l->xMin(), l->xMax(), l->yMin(), l->yMax())
< std::make_tuple(r->xMin(), r->xMax(), r->yMin(), r->yMax());
});
Copy link
Contributor

@povik povik Nov 26, 2025

Choose a reason for hiding this comment

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

While sorting here does close the discrepancy between my local runs and the CI, @gadfort points out pins_ should be having a stable order by construction. The root cause for test flakiness might lie elsewhere

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern here is that this is only masking the issue, as far as I can tell the order of these boxes matches the order they are in the database. Have you checked that the ordering of these boxes in the database differs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, why not just use the compare provided by the database for sorting

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked that the ordering of these boxes in the database differs?

I take that as sorting pins_ using the dbCompare.inc comparator and checking if it's a no-op. I'll see

Copy link
Collaborator

Choose a reason for hiding this comment

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

I took a look, the order of pins_ appears to be the same between the bazel and cmake builds. So it's likely that you are changing something with the sort, but I don't believe this is where the instability comes from.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think a much more likely culprit might be the rtree storing the shapes. This would cause the order in which we write the shapes to the database to differ because the order in the tree is different.

@maliberty
Copy link
Member

replaced by #8963

@maliberty maliberty closed this Dec 5, 2025
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.

4 participants