vtr-verilog-to-routing icon indicating copy to clipboard operation
vtr-verilog-to-routing copied to clipboard

Fixed Fc values for designs with different horizontal and vertical channel widths

Open heinerb1 opened this issue 2 years ago • 7 comments

Description

Work in progress follow up to PR #1786 that added the ability for the user to specify detailed routing for designs with different horizontal and vertical channel widths. I modified the Fc section of the build_rr_graph function and the alloc_and_load_actual_fc function which determines the actual Fc value for a particular pin. Added code in the alloc_and_load_actual_fc function that uses pin location to provide the correct Fc value using the channel width and Fc percentages specified by the user. However, the code is currently split into two sections and uses the channel widths to determine which part to use. I am beginning to work on changing this so that only one set of code is used for both cases. In addition, more documentation will need to be added and the code will need to be reviewed to determine where it needs to be improved, so there is still a decent amount of work that needs to be done.

Related Issue

#1768

Motivation and Context

Due to the changes in PR #1786, a user could now specify detailed routing for designs with different horizontal and vertical channel widths. However, due to assumptions in rr_graph that one channel would be used for the entire design, the Fc values were being computed incorrectly for the smaller of the two channel widths. In order to correct this, the rr_graph builder needed to be modified to determine which channel a pin was connecting to and calculate the correct Fc value for that pin.

How Has This Been Tested?

As mentioned above, since the code is currently split into two sections and uses the channel widths to determine which part to use, the code currently passes all tests. I have tested the new pieces with various different designs that had different and vertical channel widths, and the results seem to be correct so far. However, as additional changes are made to this code such as the unification process, it might not pass the tests and need to be modified.

Types of changes

  • [ ] Bug fix (change which fixes an issue)
  • [x] 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

heinerb1 avatar Aug 09 '21 19:08 heinerb1

The unified code is currently failing just one QoR test with the following message:

regression_tests/vtr_reg_strong/strong_clock_buf...[Fail] k6_frac_N10_mem32K_40nm_clk_buf.xml/multiclock_buf.blif/common crit_path_delay_mcw relative value 1.189550164181009 outside of range [0.9,1.1] and not equal to golden value: 1.48921

This could be due to slight differences in the new algorithm for assigning connections. These are some of the results I got from this test when comparing input connections on a memory block. Equal channel widths in the horizontal and vertical directions of 100 were used and the target fc percentage was 0.15 for these input connections.

Old Algorithm: Assigns in order Avg horizontal connections: 15.333 Avg vertical connections: 14.667 Target Fc: 15 connections New Algorithm: Assigns in order but considers pin location. Better Fc approximation for each side. Avg horizontal connections: 15.111 Avg vertical connections: 15 Target Fc: 15 connections

Pin # Direction of Channel pin Connects to # of Connections with Old Algorithm # of Connections with New Algorithm
0 Horizontal 16 16
1 Horizontal 16 16
2 Horizontal 16 16
3 Horizontal 16 16
4 Horizontal 16 16
5 Horizontal 16 14
6 Vertical 16 16
7 Vertical 16 16
8 Vertical 14 16
9 Vertical 14 14
10 Vertical 14 14
11 Horizontal 14 14
12 Vertical 14 14
13 Horizontal 14 14
14 Vertical 14 14

heinerb1 avatar Aug 09 '21 19:08 heinerb1

That QoR failure looks spurious; small design, critical path change is just outside the tolerable range. If everything else looks good, we can just widen the QoR range slightly for that test.

Restarted CI as some tests seem to have not run.

vaughnbetz avatar Aug 13 '21 00:08 vaughnbetz

@ArashAhmadian I'm going to work on getting this PR through in the next week or so. Are you working on something similar? ie, does it overlap what you are currently working on?

jgoeders avatar Oct 07 '21 14:10 jgoeders

@jgoeders @ArashAhmadian : I think we do have overlapping work here. @ArashAhmadian can you update on where you stand? I believe you are about to make a PR that goes further than this one in moving the x/y channel specifications to be independent, so this PR has likely become redundant.

vaughnbetz avatar Oct 07 '21 17:10 vaughnbetz

Sorry for the late reply, school has been super busy.

@vaughnbetz @jgoeders Yes, I think we have overlapping work. I have been working on generalizing the preamble data structures in rr_graph.cpp and rr_graph2.cpp (the ones that are passed into alloc_rr_graph) to treat x & y channels seperately. Now I have gotten to a point where I can soon make a PR, after 1) fixing a bug with global routing which may prove to be not as simple as I think it is 2) doing code clean up, testing, etc. (the usual).

Unfortunately, I don't see myself being able to make the PR anytime soon since midterms are around the corner : ( and things will get more busy after that. So I'm not sure what the best thing to do in this situation is. But I guess, a helpful initial step would be to exactly figure out what this PR is trying to achieve? I looked through the code but I'm not quite sure of why somethings are being done. I think a clarification on that would allow us to figure out what to do next.

Let me know your thoughts.

ArashAhmadian avatar Oct 09 '21 01:10 ArashAhmadian

@ArashAhmadian

Sounds good. I'm working on going through and understanding this PR, as well as getting some new students to help work on this project.

If possible, could you point me to your branch? Or create a draft PR with your branch? Even if your code isn't ready and working yet, it would be nice to see your current approach. Thanks!

jgoeders avatar Oct 14 '21 17:10 jgoeders

@ArashAhmadian

Sounds good. I'm working on going through and understanding this PR, as well as getting some new students to help work on this project.

If possible, could you point me to your branch? Or create a draft PR with your branch? Even if your code isn't ready and working yet, it would be nice to see your current approach. Thanks! @jgoeders

Sounds good. Here is the draft PR: #1883

Basically what's there so is the initial steps of breaking up the segments into horizontal and vertical ones, same with channels. We also need to modify/add some functions to support the new generalization of segments. I wont be able to revisit the PR soon, but I'd be happy to go over the approach with yourself and/or the new students once we know more about the approach in #1822.

ArashAhmadian avatar Oct 16 '21 22:10 ArashAhmadian