Skip to content

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

Open
wants to merge 101 commits into
base: master
Choose a base branch
from
Open

Add tileable RR Graph #3134

wants to merge 101 commits into from

Conversation

amin1377
Copy link
Contributor

@amin1377 amin1377 commented Jun 11, 2025

Merging OpenFPGA branch into master branch. PR #2135 explains features of OpenFPGA.

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool libarchfpga Library for handling FPGA Architecture descriptions docs Documentation lang-cpp C/C++ code libvtrutil labels Jun 11, 2025
@amin1377
Copy link
Contributor Author

amin1377 commented Jul 2, 2025

Thanks @soheilshahrouz. I’ve addressed your comments; please let me know if you have any further suggestions.

Copy link
Contributor

@soheilshahrouz soheilshahrouz left a comment

Choose a reason for hiding this comment

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

These comments apply to most files under tileable_rr_graph/ directory:

  1. integer and bool arguements are passed by const reference. They should be passed by value.
  2. Documentation comments appear at function definition. They should use doxygen style and be moved to the declartion.
  3. Block comments are used for implementation-related comments.
  4. Some range-based loops use const references to iterate over integers (ids). They should iterate by value.

@amin1377
Copy link
Contributor Author

amin1377 commented Jul 8, 2025

@vaughnbetz: I’ve addressed all the comments, and the tests are passing. If there are no further concerns, I think we can go ahead and merge this PR to begin the integration process on the OpenFPGA repo side.

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.

Hi @amin1377:
It seems that bend_start and bend_end still get reserved and resized no matter what; we should avoid allocating these if tileable rr-graphs are not in play.

The VIB grid seems higher in the source hierarchy than I'd like (see detailed comments) and we need high-level comments saying when to pay attention to it / read further.

Some other detailed comments on missing file scope or function comments.

@@ -501,6 +525,8 @@ class t_rr_graph_storage {
node_storage_.reserve(size);
node_ptc_.reserve(size);
node_layer_.reserve(size);
node_bend_start_.reserve(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

@amin1377 : looks like this hasn't been resolved ... we shouldn't be reserving these bend data structures when they aren't needed (it will create the storage).

@@ -510,6 +550,8 @@ class t_rr_graph_storage {
node_storage_.resize(size);
node_ptc_.resize(size);
node_layer_.resize(size);
node_bend_start_.resize(size);
node_bend_end_.resize(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt be resizing bend_start and bend_end when they aren't needed (only needed for tileable rr-graph)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right; I fixed the other tileable RR graph-related data structures, but this one slipped through. I've now added a flag in the RR graph storage to indicate whether the graph is tileable, and this structure is allocated only if that flag is set to true (which is done in build_tileable_unidir_rr_graph).

* Bend start and end are used to store the bend information for each node.
* Bend start and end are only used for CHANX and CHANY nodes.
* Bend start and end are only used for tileable routing resource graph.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should comment they are only allocated for tilable rr graph.

@@ -0,0 +1,388 @@
#include <cstdio>
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs a big comment saying what it is for and when it is used.

Also, I think it should be moved into the tileable rr-graph directory -- it doesn't seem fundamental enough to be in base/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tileable RR graph directory is located at route/rr_graph_generation/tileable_rr_graph and contains files related to building the tileable RR graph. The file here is related to the VIB grid. All grid-related files are located under the base directory.
If you prefer, I can create a separate directory within base to contain the VIB-related files. However, there are only a couple of VIB-related files in this directory.

const std::vector<t_segment_inf>& Segments,
std::vector<VibInf>& vib_infs);

static void process_from_or_to_tokens(const std::vector<std::string> Tokens, const std::vector<t_physical_tile_type>& PhysicalTileTypes, const std::vector<t_segment_inf> segments, std::vector<t_from_or_to_inf>& froms);
Copy link
Contributor

Choose a reason for hiding this comment

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

VIB specific functions? If so, move to a new VIB file, ideally in the tileable rr-graph directory.
Each needs a doxygen comment too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a separate file (setup_vib_utils) and moved all VIB-related functions in setup_vpr.cpp into it. In this file, only setup_vib_inf is called if the architecture specifies a VIB grid.


/**
* @brief The VIB device grid
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

At least needs a comment saying this is only used for xxx. Adding very visible state like this that most coders do not need to know about is quite bad ... push the visibility as low as you can (maybe it can't be pushed below here) but comment what it is for so people know if they have to read further or not right away.

bool shrink_boundary;

/// Allow routing channels to pass through multi-width and multi-height programmable blocks.
bool through_channel;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to control a similar thing but in a different way as the regular rr-graph generator's switchblock location pattern (https://docs.verilogtorouting.org/en/latest/arch/reference/#tag-%3Cswitchblock_locationspattern=). Makes it extra important to document that this is only for the tileable rr-graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few lines above, I specified that the following options are only for the tileable RR graph, and a few lines below the line you pointed out, I included a delimiter to indicate that the parameter list related to the tileable RR graph has ended.

@vaughnbetz
Copy link
Contributor

vaughnbetz commented Jul 9, 2025 via email

@amin1377
Copy link
Contributor Author

amin1377 commented Jul 9, 2025

Thanks @vaughnbetz for reviewing the code. I’ve addressed your comments to the best of my ability. I completely agree that we need better documentation for VIB, but I’m currently waiting on additional information from Xifan to improve the comments.

@vaughnbetz
Copy link
Contributor

Thanks @amin1377 . I think it's getting there.
But I think bend_start etc. are still always allocated? I think those should be fixed before this is merged.

@amin1377
Copy link
Contributor Author

amin1377 commented Jul 9, 2025

@vaughnbetz: I’ve added an is_tileable flag, and the data structures you mentioned are now allocated only when a tileable RR graph is created. (https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/add_tileable_rr_graph/libs/librrgraph/src/base/rr_graph_storage.h#L530-L552)

@vaughnbetz
Copy link
Contributor

vaughnbetz commented Jul 9, 2025 via email

@vaughnbetz
Copy link
Contributor

Let's talk about this PR in the VTR meeting today. I think to close it out it needs:

  • A careful check / review of the memory management of the new structures. As I listed above, I think some of the parallel vector handling has become detuned (and it should have more commenting).
  • A QoR check on the existing rr on the Titan design. We should get the same results, and (after the vector changes I mentioned above) about the same runtime and memory.

@amin1377
Copy link
Contributor Author

@vaughnbetz: I updated node_layer_, node_bend_start_, and node_bend_end_ to allocate memory in the same way as node_ptc_. One thing I wanted to mention is that I think std::vector::resize() increases the capacity by 1.5x to 2x when growing, depending on the implementation's growth policy, but I’m not entirely sure about that.

@amin1377
Copy link
Contributor Author

I'm also running this branch on Titan and will share the results once they're available.

@vaughnbetz
Copy link
Contributor

Thanks. You're right that .resize() is safe if it doubles the container size. It isn't specified by the standard as far as I can see -- there's no spec on the required amortized complexity of repeated calls to resize(size+1), unlike push_back (which does have an O(1) amortized guarantee). In practice you're right that the implementer likely doubles capacity in both cases, which would be OK.

We should at least test and comment that (you could call .capacity over an over and print it out so you can see what happens on a run). Or we could use .reserve() to guarantee, but it will be a bit more code. Our current solution is odd: we are careful with two vectors, and not careful with the others (assume the .resize will be OK).

@amin1377
Copy link
Contributor Author

@vaughnbetz: I agree, and I've applied the same approach to the rest of the vectors to match the allocation strategy used for node_ptc_.

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 libvtrutil VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants