-
Notifications
You must be signed in to change notification settings - Fork 419
Add tileable RR Graph #3134
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
base: master
Are you sure you want to change the base?
Add tileable RR Graph #3134
Conversation
…rilog-to-routing into add_tileable_rr_graph
Hi @AlexandreSinger, I think the PR is ready for your first round of review. I'd appreciate it if you could take a look. Thanks! |
Hi @soheilshahrouz, This PR is ready for your review. Since you're familiar with the RR Graph code, it would be great if you could take a look at the tileable RR Graph implementation. |
…rilog-to-routing into add_tileable_rr_graph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @amin1377 thanks for bringing this in! I recognize not all of this is your code. Overall the code is well structured however it needs some code style and data structure cleanup so it can fit in better with the rest of the VTR flow.
Some. of the data structure changes can be made into issues; however, the coding style things should probably be fixed now.
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp
Outdated
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp
Outdated
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_gsb.cpp
Outdated
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_node_builder.h
Outdated
Show resolved
Hide resolved
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_node_builder.cpp
Outdated
Show resolved
Hide resolved
…rilog-to-routing into add_tileable_rr_graph
Thanks @AlexandreSinger; reviewing this amount of code is no small task, and I really appreciate your time. I've addressed your comments. Regarding your suggestions about replacing the vector of vectors with VTR data structures: while I agree with them in principle, I didn’t apply all of them since this part of the code is not performance-critical. I think the code is now ready for the next round of reviews. I’ve also added @AmirhosseinPoolad to help with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR.
Since it's a large one, I’ve reviewed some files for now and will go through the rest soon.
…rilog-to-routing into add_tileable_rr_graph
@soheilshahrouz: Thank you for taking the time to review this PR. I’ve addressed all your comments, and I think this PR is now ready for the next round of review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Amin, thanks for fixing my prior comments. The reward for good work is more work my friend. I am just kidding; I gave this another pass focusing more on the documentation since I only glanced over it in my last review.
|
||
VIB Architecture | ||
============ | ||
The VIB architecture adds modeling support for double-level MUX topology and bent wires. In past, switch blocks have only one level of routing MUXes, whose inputs are driven by outputs of programmable blocks and routing tracks. Now outputs of programmable blocks can shape the first level of routing MUXes, while the inputs of second level involves the outputs of first level and other routing tracks. This can reduce the number and input sizes of routing MUXes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"In past" -> "In the past"
"switch blocks have" -> "switch blocks had"
"whose inputs are driven" -> "whose inputs were driven"
============ | ||
The VIB architecture adds modeling support for double-level MUX topology and bent wires. In past, switch blocks have only one level of routing MUXes, whose inputs are driven by outputs of programmable blocks and routing tracks. Now outputs of programmable blocks can shape the first level of routing MUXes, while the inputs of second level involves the outputs of first level and other routing tracks. This can reduce the number and input sizes of routing MUXes. | ||
|
||
Figure 1 shows the proposed VIB architecture which is tile-based. Each tile is composed of a CLB and a VIB. Each CLB can interact with the corresponding VIB which contains all the routing programmable switches in one tile. Figure 2 shows an example of the detailed interconnect architecture in VIB. The CLB input muxes and the driving muxes of wire segments can share the same fanins. A routing path of a net with two sinks is presented red in the Figure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea to spell out what the acronym "VIB" means in this context. "Versatile Interconnect Block (VIB)" or something just so its written down somewhere on this page.
|
||
FPGA Architecture File Modification (.xml) | ||
-------------------------- | ||
For original tags of FPGA architecture file see :ref:`fpga_architecture_description`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest: "For the original tags available for the standard FPGA architecture file see"
I feel like it can get confusing with all the architecture file words being thrown around.
For example, a length 4 wire has a bent pattern of ``- - U``. | ||
A ``-`` indicates no bent at this position and a ``U`` indicates a conterclockwise bent at the position. (``D`` indicates a clockwise bent.) | ||
|
||
.. note:: A bent wire should remain consistent in both the x and y axes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does consistent mean in this context? If you know it may be a good idea to expand on this if this is important.
.. arch:tag:: <multistage_muxs>content</multistage_muxs> | ||
|
||
:req_param content: | ||
The detaild information for first and second MUXes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"detaild" -> "detailed"
<direct name="scff_chain" from_pin="clb.sc_out" to_pin="clb.sc_in" x_offset="0" y_offset="-1" z_offset="0"/> | ||
</directlist> | ||
|
||
In OpenFPGA architecture: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"In a tileable architecture:"
|
||
In this figure, the red arrows represent the initial direct connection. The green arrows represent the point to point connection to connect all the columns of CLB. | ||
|
||
A point to point connection can be applied in different ways than showed in the example section. To help the designer implement his point to point connection, a truth table with our new parameters id provided below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"implement his point to point connection" -> "implement their point to point connection"
Anyone can be designers 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"a truth table with our new parameters id provided below". Something is off about this sentence.
@@ -0,0 +1,11 @@ | |||
#include <vector> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs a pragma once.
return node_storage_.edge_sink_node(edge); | ||
} | ||
|
||
/** @brief Get the source node for the iedge'th edge from specified RRNodeId. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a return-carriage just before the "@brief"
|
||
if (priority < max_priority_type_loc.priority) { | ||
//Lower priority, do not override | ||
#ifdef VERBOSE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if using VTR_LOG_DEBUG would be more appropriate and easier to work with.
Merging OpenFPGA branch into master branch. PR #2135 explains features of OpenFPGA.