Skip to content
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

Congestion Aware Placement #2672

Draft
wants to merge 8 commits into
base: openfpga
Choose a base branch
from
Draft

Congestion Aware Placement #2672

wants to merge 8 commits into from

Conversation

behzadmehmood-rs
Copy link
Contributor

@behzadmehmood-rs behzadmehmood-rs commented Aug 1, 2024

Embedding code for congestion aware placement for VPR.

Description

Related Issue

Motivation and Context

How Has This Been Tested?

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 Aug 1, 2024
@behzadmehmood-rs behzadmehmood-rs changed the title Cong placement Congestion Aware Placement Aug 2, 2024
@behzadmehmood-rs behzadmehmood-rs self-assigned this Aug 2, 2024
@soheilshahrouz
Copy link
Contributor

A similar PR (#2384) was submitted a year ago. If this PR supersedes #2384, please close the old one.

@behzadmehmood-rs
Copy link
Contributor Author

A similar PR (#2384) was submitted a year ago. If this PR supersedes #2384, please close the old one.

Yes, it supersedes PR (#2384).

@behzadmehmood-rs behzadmehmood-rs mentioned this pull request Aug 6, 2024
7 tasks
@vaughnbetz
Copy link
Contributor

I suggest looking at the comments in the earlier PR and addressing them -- #2384
Some of those comments are pretty important; for example there are hard-coded limits on data structures that would lead to crashes on grids of size > 400 x 400, and I think those issues are still in this PR.

@@ -54,6 +54,9 @@ enum class NetUpdateState {

const int MAX_FANOUT_CROSSING_COUNT = 50;

double cong_matrix[400][400];
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs comments on what these are, what the overall congestion modeling approach is.
The approach seems to be very similar to a placement cost function that I wrote in my PhD and called NON_LINEAR_CONGESTION. At the time I measured that code it had little gain in routability for architectures with all channels the same width (in various chip regions) and the CPU time was high. Not sure if the trade-off is different with this code, but adding QoR data on some larger circuits would be important to check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, hard-coding 400 is not good.

@behzadmehmood-rs
Copy link
Contributor Author

I suggest looking at the comments in the earlier PR and addressing them -- #2384 Some of those comments are pretty important; for example there are hard-coded limits on data structures that would lead to crashes on grids of size > 400 x 400, and I think those issues are still in this PR.

I will address those.

@behzadmehmood-rs behzadmehmood-rs marked this pull request as draft August 21, 2024 13:25
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.

6 participants