-
Notifications
You must be signed in to change notification settings - Fork 422
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
Thanks @soheilshahrouz. I’ve addressed your comments; please let me know if you have any further suggestions. |
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.
These comments apply to most files under tileable_rr_graph/ directory:
- integer and bool arguements are passed by const reference. They should be passed by value.
- Documentation comments appear at function definition. They should use doxygen style and be moved to the declartion.
- Block comments are used for implementation-related comments.
- Some range-based loops use const references to iterate over integers (ids). They should iterate by value.
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_utils.h
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
vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_node_builder.cpp
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
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
@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. |
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:
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); |
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.
@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); |
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.
Shouldnt be resizing bend_start and bend_end when they aren't needed (only needed for 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.
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. | ||
*/ |
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.
Should comment they are only allocated for tilable rr graph.
vpr/src/base/setup_vib_grid.cpp
Outdated
@@ -0,0 +1,388 @@ | |||
#include <cstdio> |
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 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/
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.
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.
vpr/src/base/setup_vpr.cpp
Outdated
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); |
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.
VIB specific functions? If so, move to a new VIB file, ideally in the tileable rr-graph directory.
Each needs a doxygen comment too.
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.
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 | ||
*/ |
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.
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; |
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 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.
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 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.
…rilog-to-routing into add_tileable_rr_graph
Thanks. I think we should push it down a directory even if it’s only a couple of files. It’s an experimental feature for routing graphs that doesn’t seem too well documented so I don’t want to burden future developers with figuring out which grid is which, unless they’re actually working on that feature.VaughnOn Jul 9, 2025, at 10:36 AM, Amin Mohaghegh ***@***.***> wrote:
@amin1377 commented on this pull request.
In vpr/src/base/setup_vpr.cpp:
+
+static void setup_routing_arch(const t_arch& Arch, t_det_routing_arch& RoutingArch);
+
+static void setup_timing(const t_options& Options, const bool TimingEnabled, t_timing_inf* Timing);
+static void setup_switches(const t_arch& Arch,
+ t_det_routing_arch& RoutingArch,
+ const std::vector<t_arch_switch_inf>& arch_switches);
+static void setup_analysis_opts(const t_options& Options, t_analysis_opts& analysis_opts);
+static void setup_power_opts(const t_options& Options, t_power_opts* power_opts, t_arch* Arch);
+
+static void setup_vib_inf(const std::vector<t_physical_tile_type>& PhysicalTileTypes,
+ const std::vector<t_arch_switch_inf>& Switches,
+ 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);
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.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
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. |
Thanks @amin1377 . I think it's getting there. |
@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) |
1. Shouldn't this resize method also have an if (is_tilable) protecting the
bend data?
https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/7544f776d2782f2f19c09d9cc7ad000f9be6a876/libs/librrgraph/src/base/rr_graph_storage.h#L561
nd
2. The node_ptc handling looks efficient to me at
https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/7544f776d2782f2f19c09d9cc7ad000f9be6a876/libs/librrgraph/src/base/rr_graph_storage.h#L532
It basically grabs the answer from make_room_in_vector and does an
equivalent reserve and resize.
**But** the node_layer and node_bend_start and node_bend_end don't look to
be doing that; they are directly calling resize, which I believe could lead
to growing a vector by one each time. They should replicate the node_ptc
code style (reserve, then resize) or just all these vectors should call
make_room_in_vector directly.
We can talk Friday if this isn't clear, but we have to be pretty
careful here not to detune memory.
Should also comment this -- the trick node_ptc is using should be
documented.
Micro-optimizations (don't have to do, but let's write them all down):
- make_room_in_vector could return a bool saying whether it did a
reallocation or not if we want to save some unnecessary calls to
reserve/reszize.
- node_bend_start and node_bend_end are separate vectors; we could put
them in a single struct instead. If we keep getting more separate vectors
the code to manage them will get messy. We do want to separate optionally
allocated data from always allocated, and hot (router) data from cold data,
but after that we can combine using structs. That would basically turn
node_bend_start, node_bend_end --> node_bend.{start,end}. Might give you
better cache locality too if you tend to ask about bend_start and bend_end
one after the other all the time.
…On Wed, Jul 9, 2025 at 5:10 PM Amin Mohaghegh ***@***.***> wrote:
*amin1377* left a comment (verilog-to-routing/vtr-verilog-to-routing#3134)
<#3134 (comment)>
@vaughnbetz <https://github.com/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
)
—
Reply to this email directly, view it on GitHub
<#3134 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNDPJ3M25ELI6WEECXRXUL3HWAK7AVCNFSM6AAAAAB7DJA7L2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTANJUGAZTQNJXHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
Let's talk about this PR in the VTR meeting today. I think to close it out it needs:
|
@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. |
I'm also running this branch on Titan and will share the results once they're available. |
…rilog-to-routing into add_tileable_rr_graph
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). |
@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_. |
Merging OpenFPGA branch into master branch. PR #2135 explains features of OpenFPGA.