Skip to content

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

Merged
merged 14 commits into from
Jun 24, 2024

Conversation

soheilshahrouz
Copy link
Contributor

@soheilshahrouz soheilshahrouz commented Jun 10, 2024

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

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Jun 10, 2024
@soheilshahrouz
Copy link
Contributor Author

Results for vtr_reg_nightly_test5/vpr_tight_floorplan test
Circuit: neuron_stratixiv_arch_timing.blif
Architectur: stratixiv_arch_neuron.timing.xml

Constraint file Total swaps Aborted swaps Placed WL Placed CPD Place time
sixteenth.xml 1.06 0.44 0.98 1.01 1.44
half_blocks_half.xml 0.98 0.54 0.90 0.89 1.31
one_big_partition.xml 1.01 0.99 0.99 1.02 1.29

@soheilshahrouz
Copy link
Contributor Author

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.

@vaughnbetz
Copy link
Contributor

@soheilshahrouz is investigating why one_big_partition slows things down -- there doesn't seem to be a good reason.

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! 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.

@soheilshahrouz
Copy link
Contributor Author

Results for vtr_reg_nightly_test5/vpr_tight_floorplan after fixing grid_loc_to_compressed_loc_approx_round_up()
Circuit: neuron_stratixiv_arch_timing.blif
Architectur: stratixiv_arch_neuron.timing.xml

Constraint file Total swaps Aborted swaps Placed WL Placed CPD Place time
sixteenth.xml 1.06 0.46 0.99 1.006 1.09
half_blocks_half.xml 0.97 0.48 0.92 0.907 0.93
one_big_partition.xml 1.01 0.99 0.99 1.023 0.95

@vaughnbetz
Copy link
Contributor

Excellent! Glad it looks like it is working now.

@vaughnbetz
Copy link
Contributor

@soheilshahrouz : we can merge if you remove the WIP from the title.
Also, did you validate the grid->compressed grid mapping code with some automated test (or extensive manual tests)? The results look good, but the new functions should still be independently tested I think, since it would be easy for a bug to sneak through and just subtly degrade optimization.

@soheilshahrouz
Copy link
Contributor Author

soheilshahrouz commented Jun 12, 2024

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.

bool cluster_floorplanning_legal(ClusterBlockId blk_id, const t_pl_loc& loc) {
auto& floorplanning_ctx = g_vpr_ctx.floorplanning();
bool floorplanning_good = false;
bool cluster_constrained = is_cluster_constrained(blk_id);
if (!cluster_constrained) {
//not constrained so will not have floorplanning issues
floorplanning_good = true;
} else {
PartitionRegion pr = floorplanning_ctx.cluster_constraints[blk_id];
bool in_pr = pr.is_loc_in_part_reg(loc);
//if location is in partitionregion, floorplanning is respected
//if not it is not
if (in_pr) {
floorplanning_good = true;
} else {
#ifdef VERBOSE
VTR_LOG("Block %zu did not pass cluster_floorplanning_check \n", size_t(blk_id));
VTR_LOG("Loc is x: %d, y: %d, subtile: %d \n", loc.x, loc.y, loc.sub_tile);
#endif
}
}
return floorplanning_good;
}

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.

bool Region::is_loc_in_reg(t_pl_loc loc) const {
bool is_loc_in_reg = false;
int loc_layer_num = loc.layer;
if (layer_num != loc_layer_num) {
return is_loc_in_reg;
}
vtr::Point<int> loc_coord(loc.x, loc.y);
//check that loc x and y coordinates are within region bounds
bool in_rectangle = region_bounds.coincident(loc_coord);
//if a subtile is specified for the region, the location subtile should match
if (in_rectangle && sub_tile == loc.sub_tile) {
is_loc_in_reg = true;
}
//if no subtile is specified for the region, it is enough for the location to be in the rectangle
if (in_rectangle && sub_tile == NO_SUBTILE) {
is_loc_in_reg = true;
}
return is_loc_in_reg;
}

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.

@vaughnbetz
Copy link
Contributor

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.

@vaughnbetz
Copy link
Contributor

Update: this looks good, but @soheilshahrouz wants to add a unit test to it before merging.

@soheilshahrouz soheilshahrouz changed the title [WIP] Fix high rejection rate for constrained blocks during annealing Fix high rejection rate for constrained blocks during annealing Jun 19, 2024
@vaughnbetz vaughnbetz merged commit 62d1243 into master Jun 24, 2024
53 checks passed
@vaughnbetz vaughnbetz deleted the update_search_range_with_floorplan_constraint branch June 24, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-cpp C/C++ code VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants