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

Specify different switch type to drive wires with different directions #2647

Merged
merged 18 commits into from
Aug 26, 2024

Conversation

saaramahmoudi
Copy link
Contributor

This PR aims to allow the architecture to specify more than one switch type to drive a pair of wires (assuming wires are uni-directional) based on the wire's direction.
This feature allows putting different delay numbers on the switch that drives the segment. For instance, an L4 wire in x-axis can go either right (incremental wire) or left (decremental wire), and we can put one mux for the first category and another with a different delay for the second.

Description

The current segment definition in the architecture file is shown below:

<segment name="L4" freq="0.80" length="4" type="unidir" Rmetal="0" Cmetal="0">
   <mux name="L4_mux"/>
   <sb type="pattern">1 1 1 1 1</sb>
   <cb type="pattern">1 1 1 1</cb>
</segment>

This PR introduces new tags called <mux_inc> and <mux_dec> to allow the user to specify different muxes for different directions (only meaningful for unidirectional segments). The usage for the same L4 wire would be:

<segment name="L4" freq="0.80" length="4" type="unidir" Rmetal="0" Cmetal="0">
   <mux_inc name="L4_mux_inc"/>
   <mux_dec name="L4_mux_dec"/>
   <sb type="pattern">1 1 1 1 1</sb>
   <cb type="pattern">1 1 1 1</cb>
</segment>

This way we can exploit the VPR data structures for RR graph generation to capture timing more accurately. These changes are backward compatible and we don't have to necessarily use the new feature.

The architecture parser will look for either a general "mux" tag or both "mux_inc" and "mux_dec" should be defined. Otherwise, it will throw an error for the user. RR graph generation would consider the switch index while creating segments, and will continue as usual.

How Has This Been Tested?

  1. I ran VTR benchmarks and generated the RR graph with both the master branch and my branch, and they looked identical. Hence, without changing the architecture file, we expect no QoR change.
  2. I used my syntax to define different muxes for incremental and decremental wires, and generated the RR graph. I used a script to check whether the switch id within each RR edge matches the one defined in the architecture file based on direction.

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)

@saaramahmoudi saaramahmoudi requested a review from vaughnbetz July 16, 2024 20:03
@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool libarchfpga Library for handling FPGA Architecture descriptions lang-cpp C/C++ code labels Jul 16, 2024
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; one commenting change suggested.
You should also add a documentation update to this PR.
Finally, you should add at least one quick-running CI test that uses the feature (you could modify an existing arch file or add a new test with a small circuit and put it in vtr_reg_strong or some equivalent).

@vaughnbetz
Copy link
Contributor

FYI, the self-hosted runners are not launching jobs right now ... not sure why. Hopefully they start soon but that's why the various _nightly runs are failing.

@github-actions github-actions bot added the docs Documentation label Jul 18, 2024
@saaramahmoudi
Copy link
Contributor Author

@vaughnbetz Thanks for the review, I update the documentation based on your suggestion. I also added two tasks, one for strong and the other for strong_odin. Please let me know if you have more suggestions for me.

@vaughnbetz
Copy link
Contributor

@AlexandreSinger has a theory about what is killing CI: this PR had a bunch of commits pushed to the branch in rapid succession which would have launched CI runs that each got cancelled by the new one. @AlexandreSinger was doing a similar thing on another PR at the same time. It seems that having a lot of rapid starts & cancellations might make the CI runners unreachable (some limit in the google cloud? some race condition?) so it would be good to try to avoid a lot of rapid pushes to the same branch that is in a PR, to see if that makes this issue go away.

@saaramahmoudi
Copy link
Contributor Author

@vaughnbetz @AlexandreSinger Sorry about that, will avoid pushing multiple commits in a branch that is a PR in the future.

@vaughnbetz
Copy link
Contributor

No problem, and it might be a red herring. Alex is trying to track this down, but it is a weird one.

@saaramahmoudi
Copy link
Contributor Author

@vaughnbetz Can you please merge this when the CI turned green?

@vaughnbetz
Copy link
Contributor

Thanks. The google cloud runners are down again, so some runs aren't going to start. Can you run vtr_reg_strong locally (I think it is on the cloud runners) and post the results to show that it passed, including the new test?

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.

Two more minor commenting updates suggested.

libs/libarchfpga/src/physical_types.h Outdated Show resolved Hide resolved
libs/libarchfpga/src/physical_types.h Outdated Show resolved Hide resolved
@saaramahmoudi
Copy link
Contributor Author

@vaughnbetz I added your new suggestions, and CIs are green (Nightly tests are also green). Could you please merge this PR?

@vaughnbetz vaughnbetz merged commit b69fa0e into master Aug 26, 2024
52 checks passed
@vaughnbetz vaughnbetz deleted the diff_switches_for_inc_dec_wires branch August 26, 2024 18:35
@vaughnbetz
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation 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.

3 participants