-
Notifications
You must be signed in to change notification settings - Fork 607
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: deleteUniqueInst #6731
base: master
Are you sure you want to change the base?
drt: deleteUniqueInst #6731
Conversation
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: bernardo <bernardoborgessandoval@gmail.com>
} else { | ||
// guarantees everyone on the family has the same unique_inst (the first | ||
// that came) | ||
unique_inst = inst_to_unique_[*unique_class.begin()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are unique_, unique_to_idx_, and inst_to_unique_ necessary?
If unique inst (class head) is always unique_class.begin(), can't the other structures be easily inferred using the inst_to_class_ map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think using actual instances to represent classes to be weird. This is something I did not caught when refactoring PA some time ago. There is actually some functions, specially in flex_pa_acc_pattern.cpp that have both the unique instance itself, as well as their idx on the vector as a parameter which is redundant at best and harmfully confusing at worst. I'm thinking of revamping the whole unique_inst structures to use the class itself to represent unique instances as I do think it is more intuitive and less error prone.
Signed-off-by: bernardo <bernardoborgessandoval@gmail.com>
Signed-off-by: bernardo <bernardoborgessandoval@gmail.com>
Supports #5867.
This PR has an ISPD and Secure CI associated an will be opened once it passes. Although
deleteUniqueInst()
has not use on this PR, it is tested by, after creating all Unique Instances incomputeUnique()
, deleting them all and adding them again.deleteUniqueInsts()
This function is responsible for deleting an unique inst and relocating all the unique insts data structures. Now that there needs to exist support for deleting unique insts, something that was not predicted by the original code, there might be a question to wether or not the current structures are the best. I've invited discussion about this structures, so here is a detailed explanation of how work, currently:
What is an Unique Instance?
Two instances of the same cell (same master) that have the same orientation and have the same offset from their preferred track pattern will have the same access pattern solution. This means that it is only necessary to solve it for one of those instances, and use that solution for the other, we say that they are the same unique instance.
A group of all instances that fall under the same unique instance is called an unique class. Currently, we elect the first instance of its class to represent the unique instance, this is referred as the head on the new code.
Here is a list of all current structures that manage unique instances:
Problems with the current structures:
Now that deleting is supported a few problem arise.
Deleting the last instance of its class:
This means that the class itself has to be deleted, implying that
unique_
will have a deletion (O(|uniques|) and thatunique_to_idx_
has to change to accommodate for that (O(|uniques|). There might be a better way to make this reciprocal data structures that are so unfriendly to deletion.Deleting the head of a class:
This means that a new head has to be elected, and that every value of
inst_to_unique_
of instance on that class have to be adjusting (O(|class|)).