drt: make guide and GR pin query order deterministic#10918
drt: make guide and GR pin query order deterministic#10918naveenvenk17 wants to merge 2 commits into
Conversation
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces deterministic sorting for guide and GR pin queries in frRegionQuery by implementing custom comparators (guideLess and grPinLess) and sorting the query results. A unit test was also added to verify the canonical ordering. The review feedback recommends optimizing these comparators by replacing expensive string comparisons (getName()) with unique integer IDs (getId()) for tie-breaking, and adjusting the unit test creation order to align with this ID-based sorting.
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
|
Updated the comparators to use object IDs and adjusted the fixture creation order. Both DRT native suites pass in the normal configuration, and the DRT lint build passes on the VM. I am also running the suites with the sanitizer runtime fix from #10911 layered underneath because the current master branch cannot link ASan binaries by itself. |
|
The sanitizer run is complete. I tested the updated branch with #10911 layered underneath so the ASan runtime can link. Both ta_unittest and gc_unittest pass with ASAN_OPTIONS=detect_leaks=0. |
osamahammad21
left a comment
osamahammad21
left a comment
There was a problem hiding this comment.
Unrelated, but original guides query has the same issue. It would be good to fix it as well, but it could be in a separate PR.
| TEST_F(TAFixture, guide_query_has_canonical_order) | ||
| { | ||
| frNet* n1 = makeNet("n1"); | ||
| frNet* n2 = makeNet("n2"); | ||
| makeGuide(n2, /*layer_num=*/2, {500, 500}, {1500, 500}); | ||
| makeGuide(n1, /*layer_num=*/2, {500, 500}, {1500, 500}); | ||
|
|
||
| initTAQueries(); | ||
|
|
||
| frRegionQuery::Objects<frGuide> result; | ||
| design->getRegionQuery()->queryGuide( | ||
| odb::Rect(0, 0, 2000, 2000), /*layerNum=*/2, result); | ||
|
|
||
| ASSERT_EQ(result.size(), 2); | ||
| EXPECT_EQ(result[0].second->getNet()->getName(), "n1"); | ||
| EXPECT_EQ(result[1].second->getNet()->getName(), "n2"); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is not directly relevant to TA testing. I would prefer to move it to a separate test focused specifically on region query.
| bool guideLess(const frGuide* lhs, const frGuide* rhs) | ||
| { | ||
| const auto [lhs_begin, lhs_end] = lhs->getPoints(); | ||
| const auto [rhs_begin, rhs_end] = rhs->getPoints(); | ||
| return std::make_tuple(lhs->getNet()->getId(), | ||
| lhs->getLayerNum(), | ||
| lhs_begin.x(), | ||
| lhs_begin.y(), | ||
| lhs_end.x(), | ||
| lhs_end.y(), | ||
| lhs->getIndexInOwner()) | ||
| < std::make_tuple(rhs->getNet()->getId(), | ||
| rhs->getLayerNum(), | ||
| rhs_begin.x(), | ||
| rhs_begin.y(), | ||
| rhs_end.x(), | ||
| rhs_end.y(), | ||
| rhs->getIndexInOwner()); | ||
| } |
There was a problem hiding this comment.
Why not sort by the ID? the GuidesProcessor assigns a global unique id for each guide before initializing the guide RQ
| if (lhs->typeId() == frcBTerm) { | ||
| const auto* lhs_term = static_cast<const frBTerm*>(lhs); | ||
| const auto* rhs_term = static_cast<const frBTerm*>(rhs); | ||
| return lhs_term->getId() < rhs_term->getId(); | ||
| } | ||
| return lhs->getId() < rhs->getId(); |
2 participants
What
Sort guide query results by net name, layer, geometry, and owner index. Sort global-routing pin results by stable instance and terminal identity.
Only results appended by the current query are sorted, so callers that accumulate results keep the existing API behavior.
Why
Boost R-tree ordering is not defined when entries have equal bounding boxes. Track assignment uses guide iteration order when it creates internal routes, and some pin handling paths select the first matching global-routing pin. Different standard-library implementations can therefore choose different routing states from identical inputs.
The new regression inserts two equal-geometry guides in reverse lexical net order. It fails on current master with
n2, n1and passes with this change asn1, n2.This addresses the remaining R-tree query-order problem described in #10336. It is independent of the priority-queue comparator work in #10816.
Testing
bazel test //src/drt/test:ta_unittest --test_filter=*guide_query_has_canonical_order*bazel test //src/drt/test:gc_unittest //src/drt/test:ta_unittestdetect_leaks=0bazel build --config=lint //src/drt:drt //src/drt/test:ta_unittestThe remaining
aes_nangate45-py_testfailure is unchanged on current master: its Python harness cannot importopenroad.