-
Notifications
You must be signed in to change notification settings - Fork 422
Fix high rejection rate for constrained blocks during annealing #2603
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
Fix high rejection rate for constrained blocks during annealing #2603
Conversation
Results for vtr_reg_nightly_test5/vpr_tight_floorplan test
|
one_big_partition.xml defines a single partition covering the entire grid. Other constraint files divide the grid into two and sixteen equally-sized regions. For these two constraint files, the number of aborted swaps is halved. This is because intersect_range_limit_with_floorplan_constraints() updates the search range of the target location, making sure tha the selected location is within the floorplan region. Without this change, the move generator might propose a target location outside the floorplanning region, causing the proposed move be aborted before cost difference evaluation starts. The placement runtime increases because proposed swaps are not aborted prematurely, and more swaps result in cost difference evalution. |
@soheilshahrouz is investigating why one_big_partition slows things down -- there doesn't seem to be a good reason. |
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! From our discussion the round_up and round_down functions may have weaknesses with large blocks and possibly a bug in general. So check those ones carefully.
…) and grid_loc_to_compressed_loc_approx_round_up()
Results for vtr_reg_nightly_test5/vpr_tight_floorplan after fixing grid_loc_to_compressed_loc_approx_round_up()
|
Excellent! Glad it looks like it is working now. |
@soheilshahrouz : we can merge if you remove the WIP from the title. |
We discussed about parital overlaps with blocks larger than 1x1 and regions. It's unclear to me what the expected behavior is when there is a partial overlap between a region and a block larger than 1x1. I'll explain my understanding of the code. Please correct me if I'm wrong. cluster_floorplanning_legal() checks if a block is within its partition region. This function calls ParitionRegion::is_loc_in_part_reg(), which then calls Region::is_loc_in_reg() for all regions in the PartitionRegion. vtr-verilog-to-routing/vpr/src/place/place_constraints.cpp Lines 213 to 240 in 9438f73
Based on Region::is_loc_in_reg(), a block location is legal if its bottom-left corner is within the floorplanning constraint. Therefore, if only the upper or right part of a block larger than 1x1 overlaps with a region, it doesn't belong to that region. vtr-verilog-to-routing/vpr/src/base/region.cpp Lines 45 to 69 in 9438f73
The newly added function for converting floorplanning regions from the device grid coordinate system to compressed grid are compatible with this understanding. I can add unit tests to check if these function are behaving as expected. |
Hmmm ... looks like I was wrong on this one. In Quartus I made floorplan regions include a block if any part was touched, but in VTR it looks like we didn't. I am now dimly recalling that this was due to Sarah finding the code more complex to handle that case, so we just decided to document it. I think we should just add this detail to the documentation. |
Update: this looks good, but @soheilshahrouz wants to add a unit test to it before merging. |
Most proposed moves for blocks with tight floorplan constraints are rejected because the proposed location for a constrained block is not withing the floorplan region. This PR tries to fix this problem.
How Has This Been Tested?
Running vtr_reg_nightly_test5/vpr_tight_floorplan regression test
Types of changes
Checklist: