Add tileable RR Graph #3134

amin1377 · 2025-06-11T18:14:24Z

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

…le info

…rilog-to-routing into add_tileable_rr_graph

amin1377 · 2025-07-02T23:16:00Z

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

soheilshahrouz

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.

libs/libarchfpga/src/physical_types_util.cpp

libs/libarchfpga/src/vib_inf.h

vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_utils.h

vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_node_builder.h

vpr/src/route/rr_graph_generation/tileable_rr_graph/tileable_rr_graph_node_builder.cpp

…rilog-to-routing into add_tileable_rr_graph

amin1377 · 2025-07-08T13:02:30Z

@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.

vaughnbetz

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.

vaughnbetz · 2025-07-08T18:13:14Z

libs/librrgraph/src/base/rr_graph_storage.h

@@ -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);


@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).

vaughnbetz · 2025-07-08T18:13:44Z

libs/librrgraph/src/base/rr_graph_storage.h

@@ -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);


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

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).

vaughnbetz · 2025-07-08T18:14:24Z

libs/librrgraph/src/base/rr_graph_storage.h

+     * 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.
+     */


Should comment they are only allocated for tilable rr graph.

vaughnbetz · 2025-07-08T18:17:03Z

vpr/src/base/setup_vib_grid.cpp

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


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/

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_vib_grid.h

vaughnbetz · 2025-07-08T18:20:39Z

vpr/src/base/setup_vpr.cpp

+                          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);


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

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.

vaughnbetz · 2025-07-08T18:22:07Z

vpr/src/base/vpr_context.h

+
+    /**
+     * @brief The VIB device grid
+     */


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.

vpr/src/base/vpr_types.h

vaughnbetz · 2025-07-08T18:25:33Z

vpr/src/base/vpr_types.h

+    bool shrink_boundary;
+
+    /// Allow routing channels to pass through multi-width and multi-height programmable blocks.
+    bool through_channel;


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.

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.

vpr/src/route/rr_graph_generation/clb2clb_directs.h

…rilog-to-routing into add_tileable_rr_graph

…raph

vaughnbetz · 2025-07-09T14:49:21Z

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: ***@***.***>

amin1377 · 2025-07-09T15:02:15Z

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 · 2025-07-09T17:57:42Z

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 · 2025-07-09T21:09:45Z

@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 · 2025-07-09T21:50:18Z

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>

vaughnbetz · 2025-07-10T16:14:52Z

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 · 2025-07-10T16:18:12Z

@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 · 2025-07-10T16:20:52Z

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

vaughnbetz · 2025-07-10T16:30:05Z

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 · 2025-07-10T16:38:50Z

@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_.

amin1377 added 19 commits June 11, 2025 09:55

[vpr][rr_graph] add tileable rr graph dir

b6371d2

[vpr][rr_graph] move tileable rr graph dir uner rr_graph_generation

77781b4

[vpr][route] update rr_graph generation with tileable rr graph

cf95e2c

[vpr][util] add is_inter_cluster_node for vib arch

431923c

[vpr][route] update router lookahead with tileable rr graph

d62fb79

[vpr][blif] use regex to find param val

02a56e9

[vpr][base] update with tileable rr graph

204a794

[vpr][route] remove unused param

7edf56d

[lib][arch] add vib processing

f5e8061

[lib][arch] add vib_inf

14064c7

[libs][rr_graph] update lib rr graph with tileable

49b04b0

[libs][rr graph][io] update read/write rr graph functions with tileab…

c6df91a

…le info

[libs][rr graph][io] update rr_grpah utils with tileable info

bf19150

[lib][rr_graph] add vtr tokenizer

3970db7

[lib][util] update capnp and util with open fpga

797b488

add openfpga doc

cf2dbc8

[vpr][base] add vib grid

0e03dc7

add VIB doc

f8fae78

[vpr][route] fix alloc_and_load_rr_switch_inf definition

5462ccd

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 added 6 commits June 11, 2025 14:15

make format

7cdf5bf

[vtr_flow][test] add openfpga arch

5ad8303

[CI] add openfpga tests

c79ab8f

fix formatting

8eccbcc

Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…

0cc6f10

…rilog-to-routing into add_tileable_rr_graph

[doc][arch] add tileable doc

5a492c8

soheilshahrouz requested changes Jul 4, 2025

View reviewed changes

amin1377 added 6 commits July 7, 2025 20:05

[vpr][tileable] move comments from cpp to .h + fix formatting

e03a84d

[vpr][tileable] fix commenting style

556d499

[vpr][tileable] remove const reference to ids

ec190c3

Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…

78b4342

…rilog-to-routing into add_tileable_rr_graph

make format

c6d641d

[libs][arch] move e_parallel_axis to logic_types

1486073

vaughnbetz requested changes Jul 8, 2025

View reviewed changes

amin1377 added 5 commits July 9, 2025 09:19

Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…

0a6eb59

…rilog-to-routing into add_tileable_rr_graph

make format

8c1729e

[libs][rr_graph] add is_tileable to rr_graph_storage

262ab69

[route][rr_grpah] set is_tileable to true when building tileable rr g…

477103a

…raph

[vpr][base] add setup_vib_utils

5e24230

[vpr][base] create a new diretory for vib-specific grid

b6becb2

amin1377 added 2 commits July 9, 2025 16:34

update check_route for MUX type

1a7986a

[rr_graph] raise error if chanxy min loc is less than zero

7544f77

[libs][rr_graph] reserve capacity for node_bend_

907565a

Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…

865b4bb

…rilog-to-routing into add_tileable_rr_graph

Add tileable RR Graph #3134

Are you sure you want to change the base?

Add tileable RR Graph #3134

Uh oh!

Conversation

amin1377 commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amin1377 commented Jul 2, 2025

Uh oh!

soheilshahrouz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amin1377 commented Jul 8, 2025

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vaughnbetz commented Jul 9, 2025 via email

Uh oh!

amin1377 commented Jul 9, 2025

Uh oh!

vaughnbetz commented Jul 9, 2025

Uh oh!

amin1377 commented Jul 9, 2025

Uh oh!

vaughnbetz commented Jul 9, 2025 via email

Uh oh!

vaughnbetz commented Jul 10, 2025

Uh oh!

amin1377 commented Jul 10, 2025

Uh oh!

amin1377 commented Jul 10, 2025

Uh oh!

vaughnbetz commented Jul 10, 2025

Uh oh!

amin1377 commented Jul 10, 2025

Uh oh!

Uh oh!

amin1377 commented Jun 11, 2025 •

edited

Loading