Skip to content

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

Merged
merged 69 commits into from
Oct 12, 2021
Merged

Conversation

sfkhalid
Copy link
Contributor

@sfkhalid sfkhalid commented Sep 9, 2021

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

@github-actions github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Sep 9, 2021
…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
@sfkhalid
Copy link
Contributor Author

sfkhalid commented Oct 5, 2021

Thanks @sfkhalid . I think both placement time and (fixed channel width) critical path routing runtime are down by 2%? Is that right? Probably just machine load noise then. The total flow time changed more, but is probably dominated by the binary search runtime, which is more chaotic. Please take a look at the place time in particular to confirm it is OK.

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.

@vaughnbetz
Copy link
Contributor

OK, that sounds fine.

@sfkhalid
Copy link
Contributor Author

sfkhalid commented Oct 6, 2021

New titan QoR comparison (after fixing bug related to syncing physical pins during placement):
titan_qor_comparison.xlsx
No significant changes in place time

Copy link
Contributor

@vaughnbetz vaughnbetz left a 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.

@sfkhalid
Copy link
Contributor Author

sfkhalid commented Oct 8, 2021

@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.
nightly1_with_changed_min_chan_width_route_time.log

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?

@sfkhalid
Copy link
Contributor Author

@vaughnbetz Does this look good to merge?

@vaughnbetz
Copy link
Contributor

Looks good, thanks. Merging.

@vaughnbetz vaughnbetz merged commit 41e0879 into master Oct 12, 2021
@vaughnbetz vaughnbetz deleted the change_place_flow_in_init_place branch October 12, 2021 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants