Skip to content
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.

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

drt: nodes structure refactoring and elimination of getFlatIdx #5903

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

bnmfw
Copy link
Contributor

@bnmfw bnmfw commented Oct 9, 2024

This PR has a secure CI associated with it, is branched from #5901 and is waiting for it to merge.

Nodes structure change

The nodes vector has been refactored to be a vector of vectors index by the old "nested id". This allowed for the elimination of the getFlatIdx() function, as now the indexing for nodes is always done through both idx.

This change impacts various parts of the code as the structure is present in most of the functions that work with Access Patterns.

getEdgeCost refactoring

This PR also contains some minor refactoring on getEdgeCost (both implementations).

The refactoring that does not have to do with the nodes restructuring are:

  • Returning the edge cost directly to allow for use of guards
  • On the long getEdgeCost implementation both checks for violation edges were unified.

Do not try to look at changes, it currently has 4 PRs worth of contents, I just want to make it to organize myself and see the public CI.

Copy link
Contributor

github-actions bot commented Oct 9, 2024

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

Copy link
Contributor

github-actions bot commented Oct 9, 2024

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

@bnmfw bnmfw changed the title drt: nodes structure refactoring drt: nodes structure refactoring and elimination of getFlatIdx Oct 9, 2024
@bnmfw bnmfw mentioned this pull request Oct 14, 2024
Copy link
Member

@osamahammad21 osamahammad21 left a comment

Choose a reason for hiding this comment

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

I think it is cleaner to go with one of those 2 choices:

  • use vector of unique pointers and reference the objects by their pointers.
  • the current vector of object but reference by index or object reference.

src/drt/src/pa/FlexPA.h Show resolved Hide resolved
src/drt/src/pa/FlexPA.h Show resolved Hide resolved
/*either {pin_idx, acc_point_idx} or {inst_idx, acc_pattern_idx} depending on
* context*/
std::pair<int, int> idx_ = {-1, -1};
FlexDPNode* prev_node_ = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

store prev_node_idx_ instead of pointer to FlexDPNode. safer to avoid memory reallocation causing dangling pointers. (or use vector<vector<unique_ptr>>)

src/drt/src/pa/FlexPA_prep.cpp Show resolved Hide resolved
src/drt/src/pa/FlexPA_prep.cpp Show resolved Hide resolved
src/drt/src/pa/FlexPA_prep.cpp Show resolved Hide resolved
src/drt/src/pa/FlexPA_prep.cpp Show resolved Hide resolved
src/drt/src/pa/FlexPA_prep.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants