-
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
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.
|
||
.. note:: These options are required | ||
|
||
In the OpenFPGA architecture file, you may define additional attributes for each VPR's direct connection: |
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.
Are we destinguishing OpenFPGA architecture files from regular architecture files?
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.
Based on the meeting we had before, I guess the conclusion was to not support all OpenFPGA features in the standard RR Graph generation (at least for now). As a result, I’m creating a separate section to clearly outline the additional features that are exclusively supported by the tileable RR Graph.
doc/src/vpr/VIB.rst
Outdated
@@ -0,0 +1,254 @@ | |||
.. _VIB: | |||
|
|||
VIB 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.
What does VIB. I think this file also needs some context. Why is it here? Who should be using this 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.
Good point. I moved it under the Tileable Architecture section. The vib.rst file explains what VIB is. In short, VIB combines the connection block (CB), switch block (SB), and intra-block crossbar into a single block. As a result, each tile consists of two sub-blocks: the functional block and the VIB (Versatile Interconnect Block). This design facilitates switch architecture exploration and ideally improves router flexibility.
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. |
Merging OpenFPGA branch into master branch. PR #2135 explains features of OpenFPGA.