-
Notifications
You must be signed in to change notification settings - Fork 415
Change place flow in init place #1841
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
Conversation
…d to check whether macro had already been placed. Added check.
…re sorted in, rather than placing all macros and then all non-macro blocks
…ocations data structures
…not root locations and added routine to echo compressed grid
…l placement, and made macros also not use free_locations and legal_pos
…d grid rather than regular grid
…e when the assertion level is 4. Doing this so I can run on CI with these messages and see if this is the assertion failure that is causing the strong failures
Yes, that's correct. On the sheet called compare_times, I have a comparison of the place times and they are basically the same. The min_chan_width_route_time is what's making the overall flow time go up, because that value went up by 24% when comparing the changed branch to the master. I'll also take a look at the initial placement times to make sure they're the same. |
OK, that sounds fine. |
New titan QoR comparison (after fixing bug related to syncing physical pins during placement): |
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.
Looks good; just a few suggestions for shorter/more clear code in a few places.
…ts updated for that circuit - want to check if all tests otherwise pass
@vaughnbetz All the tests have passed on this PR - does it look good to merge? I wanted to mention some changes to the golden results I made for the reg test in nightly_test1 - arithmetic_tasks/multless_consts. I had to update the golden results for this test, and when I did, a few of the circuits started having the min_chan_width_route_time relative value go up to around 12-13. I attached the log file below so you can see which circuits this happens to. To counteract this, I changed the pass requirement range for min_chan_width_route_time to [0.1, 14.0] (before it was [0.1, 10.0]). The range was already quite wide to begin with so I thought stretching it more would be an ok solution, since the min_chan_width_route_time is a value that changes a lot. I compared the min chan width route time values from the previous golden result to the new golden result and they don't seem to change that drastically either. For example, for the circuit mult_093, it went from 3.21 to 1.14, for the circuit mult_051 it went from 3.79 to 1.59. My question is, would it be better to go in and manually make the values in the golden results closer to what they used to be (so they would be a bit higher for these circuits) so that I don't have to widen the range for min_chan_width_route_time to pass? Or is this current solution I have sufficient? |
@vaughnbetz Does this look good to merge? |
Looks good, thanks. Merging. |
During initial placement, all macros were placed before all blocks. On this pull request, that flow was changed. Now, all blocks (whether part of a macro or not) are placed in the order that they were sorted in - blocks are sorted from most difficult to place to least difficult to place.
Also, initial placement no longer uses the free_locations and legal_pos data structures to find potential locations for the blocks. Instead, it now uses the compressed grids to search for potential locations for each block. It tries up to a maximum number of randomly-chosen locations from within the block's floorplan region (the floorplan region is the whole chip if the block is not constrained) and after that turns to exhaustive placement if the block still has not been placed.
Related Issue
Related to adding placement constraints to vpr #932