-
Notifications
You must be signed in to change notification settings - Fork 412
[WIP] Change GreedySeedSelector to work with molecules instead of atoms #3024
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
base: master
Are you sure you want to change the base?
Conversation
In hindsight it wasn't correct to say that it generates exactly the same packing as it could change the order of seeds that have the same gain since I'm using molecule ids and not atom ids. It doesn't make any noticeable difference other than a single strong_odin test that resulted in MCW of 14 instead of 12. |
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.
LGTM! This is a very very welcome change! I was the one who wrote the TODO for this and I am happy with your implementation. The reason I did not do this originally is due to what you found; it can cause changes due to ordering. I am happy to see that it only changed one circuit!
We will need to run the nightly tests as well on this branch before it can be merged. There are some small testcases in the nightly tests which may fail due to see ordering.
Regarding the Odin testcase, I think its safe to just regenerate the golden result for that one. |
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.
Leaving a bunch of notes for myself here, will fix later.
ec0d4dc
to
b2deb8f
Compare
Ran the nightly tests today. There was a single QoR failure in NightlyTest5:
It makes no sense for pack time to change this much with such a simple change that should, if anything, speed things up a tiny bit since there would be less look ups between atoms and molecules during packing. I will re-run that circuit again and compare to master to confirm if there's an issue or if it was simply machine load. @AlexandreSinger FYI |
What can happen in this case is that the golden results were very old and were just on the edge of being too slow. Due to small run-time differences in your PR it may have gone over the edge. I would recommend just re-generating the golden results for this one testcase to be safe. That should re-center the golden results to keep this from happening again. I am not too worried about smaller tests like this one increasing in run time. |
I just did a comparison with master and it's really struggling to pack DSPs. In master this circuit fails packing in the first iteration because of LAB floorplan constraints but it gets successfully packed in the next iteration. With this change there's also a whole bunch of DSP overfilled regions and goes through a lot more iterations to be packed. It seems like this needs a little bit more work. |
Unrelated to this PR, the golden results are generated on Wintermute but we're running CI on Savi 2. Should we regenerate golden results of VTR 9 on Savi 2 and update them? |
709d23b
to
f9d543c
Compare
f9d543c
to
b7af89d
Compare
This PR changes the GreedySeedSelector class to work with molecules instead of atoms. Before this, GreedySeedSelector has a list of atoms sorted by their gain. It now has a list of molecules sorted by their gain.
Since seeds were selected ordered by their gain, it meant that effectively each molecule's gain was equivalent to the maximum gain of its atoms. The calculated molecule gain here is also the maximum atom gain of a molecule and should produce equivalent packing. I also ran LU8EEng from VTR benchmarks and I indeed got the exact same results (number of blocks, cpd and total wirelength) as before these changes.