Skip to content

[Place] Expand search range for sparse blocks #2960

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

Open
wants to merge 60 commits into
base: master
Choose a base branch
from

Conversation

amin1377
Copy link
Contributor

@amin1377 amin1377 commented Apr 1, 2025

This PR addresses Issue #2959. The solution to fix the problem is a bit different from the one stated there, though. To ensure moving sparse blocks (e.g., IO blocks), we expand the search range to include the whole column if the number of compatible blocks in the given column is less than a certain threshold (currently, this number is set to 3)

The above update changed the placement of the top picture to the placement of the bottom one (where there is only one IO block left on the top).
Screenshot 2025-04-01 151404


Screenshot 2025-04-01 150305

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Apr 1, 2025
@amin1377 amin1377 requested a review from vaughnbetz April 1, 2025 19:37
Copy link
Contributor

@soheilshahrouz soheilshahrouz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the implementation can be made more modular by adding another function to modify the search limit instead of modifying when you're searching for a compatibale location.

@soheilshahrouz
Copy link
Contributor

Does the described problem happen only when IO blocks are located at the top/bottom rows? What if they are initially placed at left/right sides of the device?

@amin1377
Copy link
Contributor Author

amin1377 commented Apr 1, 2025

Does the described problem happen only when IO blocks are located at the top/bottom rows? What if they are initially placed at left/right sides of the device?

I'm not entirely sure, but given that the x-axis of the compressed grid is fully dense, I don't think this happens for the x-axis.

@amin1377
Copy link
Contributor Author

amin1377 commented Apr 2, 2025

Does the described problem happen only when IO blocks are located at the top/bottom rows? What if they are initially placed at left/right sides of the device?

I'm not entirely sure, but given that the x-axis of the compressed grid is fully dense, I don't think this happens for the x-axis.

Upon further discussion with @soheilshahrouz, it seems likely that the same issue occurs along the x-axis. So, it would make sense to expand the x-axis range limit if the number of compatible blocks in a given row falls below a certain threshold. Otherwise, there’s a bias toward placing IO blocks on the top and bottom edges rather than on the left and right.

I’ll look into this in a separate PR.

@tpagarani FYI

@amin1377
Copy link
Contributor Author

amin1377 commented Apr 3, 2025

@soheilshahrouz: I’ve addressed all your comments and implemented the changes you requested. Since the code has changed significantly since your last review, I’d appreciate it if you could take another look. Thanks!

@vaughnbetz
Copy link
Contributor

You should add QoR data on a couple of big benchmark suites.

@vaughnbetz
Copy link
Contributor

Also summarize the QoR-related regtest failrues (basic has two failing QoR, but it is because the wirelength improved a lot (45%) on two small circuits which is certainly fine!
I'd run at least one more benchmark suite if Titan shows some degradation, to see if it's real or not. We can accept a degradation and retune if needed but I'd like to understand it.

@vaughnbetz
Copy link
Contributor

Results on 3 seeds show the cpd degradation isn't consistent (the other 2 seeds were fine). I think this is good to merge. Can you also add a link to the 3 seed data here for posterity @amin1377 ?

@vaughnbetz
Copy link
Contributor

It looks like some golden results need to be updated. I looked at the parmys basic failure and it is a single small design that improved too much, so it's not a problem (actually it's good news):
[Fail]
k6_frac_N10_mem32K_40nm.xml/multiclock_separate_and_latch.v/common routed_wirelength relative value 0.4 outside of range [0.6,1.5], above absolute threshold 5.0 and not equal to golden value: 10.0

amin1377 added 18 commits May 14, 2025 16:36
This reverts commit 96e9cc5.
…tions in initial placement to prevent search range to be adjusted"

This reverts commit ade994b.
…find_compatible_compressed_loc_in_range"

This reverts commit f9e8517.
@amin1377 amin1377 closed this May 27, 2025
@amin1377 amin1377 reopened this May 27, 2025
@amin1377
Copy link
Contributor Author

amin1377 commented May 27, 2025

@vaughnbetz:

I updated the code to expand the search range if the number of blocks in the selected column is less than 3. I ran both the Titan benchmarks and the VTR large set with 5 seeds (note that runtime isn't reliable since I ran it on Wintermute).

Overall, the QoR hasn't changed significantly. I compared the results circuit by circuit, and for those where the default seed (seed 1) showed a significant QoR increase, there were other seeds where the QoR improved noticeably. I’ve applied color formatting to the "ratio" tab so you can easily see the results on a per-circuit basis.

Titan: Link
VTR: Link

@amin1377
Copy link
Contributor Author

@soheilshahrouz

I had to revert the commit that implemented your suggestion about adjusting the search range before calling find_compatible_compressed_loc_in_range, since I need to know which column is selected to expand the search range accordingly.

@vaughnbetz
Copy link
Contributor

LGTM, thanks!

@vaughnbetz
Copy link
Contributor

There are a few CI failures. I looked at the strong failures and it is 4 small circuits with larger than bounds QoR changes, all of which look harmless. So you can check in new golden results, or widen the ranges for those smaller tests if you prefer. Then it's good to merge.

@amin1377
Copy link
Contributor Author

There are a few CI failures. I looked at the strong failures and it is 4 small circuits with larger than bounds QoR changes, all of which look harmless. So you can check in new golden results, or widen the ranges for those smaller tests if you prefer. Then it's good to merge.

Thanks, Vaughn! I'm running the nightly tests to check if there are any results that need updating. I'll merge the PR afterward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-cpp C/C++ code libarchfpga Library for handling FPGA Architecture descriptions VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants