Skip to content

librrgraph: add correct array operator #3306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

librrgraph: add correct array operator #3306

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions libs/librrgraph/src/base/rr_graph_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,12 @@ class edge_sort_iterator {
return lhs.swapper_.idx_ <= rhs.swapper_.idx_;
}

const t_rr_edge_info operator[] (ssize_t n) const {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
const t_rr_edge_info operator[] (ssize_t n) const {
t_rr_edge_info operator[] (ssize_t n) const {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some early testing reports from our next integration hint at needing to remove the const. We will get back to you.

edge_sort_iterator ret = *this;
ret.swapper_.idx_ += n;
return ret.swapper_;
}

Comment on lines +238 to +243
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this compile successfully on your system if it returned an edge_swapper& like operator* does? operator[n] is (or should be) equivalent to *(it + n). We basically need a combination of operator+ and operator* here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think the previous PR was fine. The function was const and that was correct since (*this + n) constructs a new iterator and doesn't modify 'this' at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

!3298 did not work as it still led to segfaults, so Amin reverted it.

ToT stands for Top of Tree, sorry about that. The newer compilers appear to be more strict.

The current PR as is, has now been tested successfully across a wide variety of compilers and systems (gcc, llvm, nvidia/intel/amd/cray/ibm compilers, and their respective hardware). SPEC CPU 2026 is built with -std=c++17; builds with newer standards might work but we are focusing on C++17 as the baseline. The successful tests are NOT using upstream VPR's unit test or regressions, but rather just the few titan cmdlines of place+route that we run for the benchmarks.

I'm not a tester, I'm just the project leader and messenger 😄. Maybe the community waits to merge this until the problem is exposed by a more mature gcc-16, and maybe the real solution is to use ranges::zip_view. I don't have an opinion on whether or not this change should be accepted here, I am just sharing that it works for SPEC. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our compiler team did a bisect and said that the "first bad commit" on gcc-16 was the following, so maybe that gives a clue to the behavioral change for VPR which necessitates this PR.
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=aaeca77a79a9a897c97f00f83f2471e7a8bd6685

RREdgeId edge() const {
return RREdgeId(swapper_.idx_);
}
Expand Down