Refactor odb ptr sets and maps to be deterministic and not use std::less#10398
Refactor odb ptr sets and maps to be deterministic and not use std::less#10398calewis wants to merge 27 commits into
Conversation
To avoid non-deterministic behavior caused by pointer comparisons, this change introduces a new header OdbPtrSetMap.h which defines ODBPtrLess (comparing by ID) and aliases OdbPtrSet and OdbPtrMap. It also modifies dbCompare.inc to explicitly delete specializations of std::less for all dbObject-derived types. This forces the use of the new aliases or custom comparators. Signed-off-by: Drew Lewis <cannada@google.com>
Refactored usage of std::set and std::map with ODB pointer keys to use custom OdbPtrSet and OdbPtrMap to avoid issues with deleted std::less. TAG=agy CONV=e06fe1f4-ec0b-4f12-8531-168335e1a959 Signed-off-by: Drew Lewis <cannada@google.com>
Signed-off-by: Drew Lewis <cannada@google.com>
Added fallback to ifp.site_set if openroad.site_set is not available, resolving AttributeError in CMake builds where Python bindings are modular. TAG=agy CONV=7a636c20-220b-489a-b173-266014ed9ffd Signed-off-by: Drew Lewis <cannada@google.com>
|
@QuantamHD can you check this for swig/python stuff, I'm not really all that sure how those interfaces work. |
…ormat violation Upstream clang-format (version 18.1.3) complained about the alignment of parameters in combineMapData. My local version (Google) did not make any changes, so I manually reformatted it to use broken lines with indentation to satisfy both. TAG=agy CONV=7a636c20-220b-489a-b173-266014ed9ffd Signed-off-by: Drew Lewis <cannada@google.com>
This matches the upstream formatter and resolves the violation by putting = 0; on a new line while keeping parameters aligned. TAG=agy CONV=7a636c20-220b-489a-b173-266014ed9ffd Signed-off-by: Drew Lewis <cannada@google.com>
![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 refactors the codebase to replace standard std::set and std::map containers that use odb::dbObject* keys with odb::OdbPtrSet and odb::OdbPtrMap aliases. This change ensures deterministic iteration order by using odb::ODBPtrLess (which compares object IDs) instead of non-deterministic pointer addresses. I have identified a duplicate include in src/dpl/include/dpl/Opendp.h and suggested using the new aliases in src/ifp/include/ifp/InitFloorplan.hh and src/pdn/include/pdn/PdnGen.hh for consistency. Additionally, I questioned the necessity of the ordered_nets_ member variable in GlobalRouter.h given the deterministic nature of the new map containers.
Signed-off-by: Drew Lewis <cannada@google.com>
Signed-off-by: Drew Lewis <cannada@google.com>
… files Signed-off-by: Drew Lewis <cannada@google.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: Drew Lewis <cannada@google.com>
Signed-off-by: Drew Lewis <cannada@google.com>
- Break line in via.h before `= 0` to fix clang-format violation. - Apply black formatting suggestions to init_floorplan_flip_sites.py and init_floorplan_gap.py. Signed-off-by: Drew Lewis <cannada@google.com>
Used Docker (Ubuntu 24.04) to run clang-format 18.1.3 on src/rsz/src/SwapArithModules.hh to fix violations, as local clang-format versions were not working correctly. TAG=agy CONV=c4c586ce-ce7f-4a27-abb1-99ad4b9ef37a Signed-off-by: Drew Lewis <cannada@google.com>
Formatted src/odb/include/odb/dbCompare.inc using clang-format-18 to resolve differences produced by the code generator. TAG=agy CONV=c4c586ce-ce7f-4a27-abb1-99ad4b9ef37a Signed-off-by: Drew Lewis <cannada@google.com>
|
C++ never ceases to surprise. For posterity Claude explains your example: The reason comes down to one rule in the standard: specializations of standard library templates must depend on a program-defined type ([namespace.std]). The key question is whether Goose* counts as program-defined. Per [defns.prog.def.type] (C++20+), a program-defined type is a non-closure class type or enumeration type that is not part of the standard library and not implicitly defined by the implementation. Pointer, reference, array, and function types are explicitly not program-defined types, even when they're built out of program-defined types. So Goose qualifies, but Goose* does not. |
|
Does deleting actually work if the compiler is freely ignoring these methods? |
Glad it figured this out now, different models including different Anthropic models weren't necessarily able to figure this one out. I do wonder if you don't tell it this is UB and you just ask it "is this legal" if it will come to the same conclusion. |
calewis
force-pushed
the
refactor-odb-ptr-sets-jetski
branch
from
ee6ae92 to
21d579d
Compare
|
fwiw I asked "explain why this is non-deterministic" and pasted your example (Opus 4.7 Adaptive) |
calewis
force-pushed
the
refactor-odb-ptr-sets-jetski
branch
from
21d579d to
dd7cd6b
Compare
Renamed OdbPtrSet and OdbPtrMap to PtrSet and PtrMap across the entire codebase. Also renamed the OdbPtrSetMap.h header file to PtrSetMap.h and updated all includes and build definitions. TAG=agy CONV=9aa3f328-c1ec-4454-a257-bc93f42fe188 Signed-off-by: Drew Lewis <cannada@google.com>
calewis
requested review from
AcKoucher,
eder-matheus,
gudeh,
maliberty and
precisionmoon
- Refactored InitFloorplan (ifp) and PdnGen (pdn) C++ APIs and implementations to use odb::PtrSet and odb::PtrMap instead of raw std::set/std::map with ODBPtrLess. - Updated Python SWIG interfaces (InitFloorplan-py.i and PdnGen-py.i) to include PtrSetMap.h. - Used SWIG %apply directives to map odb::PtrSet and odb::PtrMap to the instantiated std::set/std::map templates, ensuring proper Python typemaps and full backwards compatibility. Signed-off-by: Drew Lewis <cannada@google.com>
| try: | ||
| flipped_sites = openroad.site_set() | ||
| except AttributeError: | ||
| import ifp | ||
|
|
||
| flipped_sites = ifp.site_set() | ||
| flipped_sites.insert(site) |
There was a problem hiding this comment.
This is a bit ugly, why is the try need?
There was a problem hiding this comment.
Different namespaces (whatever python calls them) in the cmake and bazel builds. I can try to figure it out, I just don't know much about swig and the python interface.
There was a problem hiding this comment.
Once we move to bazel only we can simplify this.
There was a problem hiding this comment.
@maliberty I was able to fix this, but I needed to delete some native py_tests in the bazel build. The tests are still there as part of the regression suite:
> bazel query 'tests(//src/pdn/test/...)' | grep py_test
Loading: 0 packages loaded
//src/pdn/test:asap7_taper-py_test
//src/pdn/test:core_grid-py_test
//src/pdn/test:core_grid_with_rings-py_test
//src/pdn/test:core_grid_with_routing_obstructions-py_test
//src/pdn/test:existing-py_test
//src/pdn/test:macros-py_test
//src/pdn/test:max_width-py_test
//src/pdn/test:min_width-py_test
//src/pdn/test:offgrid-py_test
//src/pdn/test:pdn_readme_msgs_check-py_test
//src/pdn/test:power_switch-py_test
//src/pdn/test:repair_vias-py_test
//src/pdn/test:report-py_test
//src/pdn/test:reset-py_test
//src/pdn/test:ripup-py_test
//src/pdn/test:widthtable-py_testBut the versions that look like: //src/pdn/test:asap7_taper are gone. The issue was that those native tests didn't pick up the shims from test/bazel_test.sh and thus didn't have site_set in the namespaces (like pdn) that would need it. Is this sufficient or would you like me to keep iterating on it?
|
I've looked through the whole PR and it is mostly mechanical changes so I don't think it needs review from all module owners. |
Signed-off-by: Drew Lewis <cannada@google.com>
…sets-jetski Signed-off-by: Drew Lewis <cannada@google.com>
- Add 'ifp' and 'pdn' to the list of dynamically generated Python shims in 'test/bazel_test.sh'. This allows Bazel integration tests to use the tool-specific namespaces (e.g., 'import ifp', 'import pdn') seamlessly, matching the CMake namespace behavior. - Remove try-except namespace fallback workarounds from 'init_floorplan_flip_sites.py', 'init_floorplan_gap.py', and 'pdn_aux.py'. - Remove redundant native 'py_test' targets and 'helpers' py_library from 'src/pdn/test/BUILD'. These duplicate targets are already 100% covered by the 'regression_test' macro targets (e.g., ':asap7_taper-py_test') which correctly utilize the test runner's shim environment. TAG=agy CONV=56950afb-bfb2-4074-bfcb-927c5a0f1057 Signed-off-by: Drew Lewis <cannada@google.com>
- Remove unused header <fstream> from 3dblox.cpp. - Remove unused header <iterator> from dbModNet.cpp. TAG=agy CONV=56950afb-bfb2-4074-bfcb-927c5a0f1057 Signed-off-by: Drew Lewis <cannada@google.com>
Head branch was pushed to by a user without write access
TAG=agy CONV=56950afb-bfb2-4074-bfcb-927c5a0f1057 Signed-off-by: Drew Lewis <cannada@google.com>
- Enable parse_headers in root BUILD.bazel. - Enable layering_check in test/orfs/mock-array/BUILD. - Remove unused py_library load from src/pdn/test/BUILD. TAG=agy CONV=56950afb-bfb2-4074-bfcb-927c5a0f1057 Signed-off-by: Drew Lewis <cannada@google.com>
- Add missing blank lines after 'import ifp' in init_floorplan_flip_sites.py and init_floorplan_gap.py to satisfy black formatting checks. TAG=agy CONV=56950afb-bfb2-4074-bfcb-927c5a0f1057 Signed-off-by: Drew Lewis <cannada@google.com>
| #pragma once | ||
|
|
||
| #include <map> | ||
| #include <set> |
There was a problem hiding this comment.
warning: included header functional is not used directly [misc-include-cleaner]
| #include <set> | |
| #include <map> |
|
(drive-by comment: At some point it would be neat to address the annoying off-by-one diffs that clang-tidy spits out in the CI) |
3 participants
std::lessspecializations are not guaranteed to be used for pointers (https://godbolt.org/z/MedaKx79M for proof). This was causing non-determism when building with libc++ at Google. This is likely also the cause of non-determinism on Macs, but I didn't check a mac build to confirm that this fixes the issue.To fix we introduce a new alias that uses a custom comparison (the same one used by the old
std::lessspecialization) and we delete thestd::less<T>::operator()functions causing the build to fail if someone tries to make amaporsetusingstd::less<>.