Move node_tilable_track_nums_ to rr_graph_storage and add remove_node #3347

amin1377 · 2025-11-17T23:24:07Z

This PR depends on PR #3342. Please wait until that PR is merged.

In this PR, node_tilable_track_nums_ is moved into the RR graph storage instead of residing in the RR graph builder, since this data structure (when using a tileable RR graph) needs to remain valid for the entire lifetime of the RR graph.

Additionally, a method is added to facilitate removing nodes from the RR graph.

…to-routing/vtr-verilog-to-routing into remove_node

…ile if graph type is tileable

…to-routing/vtr-verilog-to-routing into remove_node

AmirhosseinPoolad · 2025-11-18T00:19:05Z

What's the use case of removing nodes?

amin1377 · 2025-11-18T12:30:16Z

What's the use case of removing nodes?

For the custom RR graph, in the other PR there are channel nodes, particularly around the perimeter of the device, that don’t have any fan-in. We need to remove these nodes after the graph is created. I considered not adding them in the first place, but that’s difficult to determine before the edges are added.

As we discussed earlier, I’m trying to break the custom RR graph PR into multiple smaller PRs to make the review process easier.

…rilog-to-routing into remove_node

…to-routing/vtr-verilog-to-routing into remove_node

…rilog-to-routing into remove_node

AmirhosseinPoolad

Thanks for the PR Amin. Had some comments.

AmirhosseinPoolad · 2025-11-20T20:59:18Z

libs/librrgraph/src/base/rr_graph_builder.cpp

            if (e_rr_type::CHANX == node_type || e_rr_type::CHANY == node_type) {
-                ptc = (node_type == e_rr_type::CHANX) ? node_tilable_track_nums_[node][x - node_storage_.node_xlow(node)] : 
-                    node_tilable_track_nums_[node][y - node_storage_.node_ylow(node)];
+                const std::vector<short>& track_nums = node_storage_.node_tilable_track_nums(node);
+                ptc = (node_type == e_rr_type::CHANX) ? track_nums[x - node_storage_.node_xlow(node)] : 
+                      track_nums[y - node_storage_.node_ylow(node)];


I really don't like the way the ternary operator is used here. We have an assertion at the beginning of this function is CHANX or CHANY, so I think we could do something like:

ptc_num = undefined if (chanx) ptc_num = something else if (chany) ptc_num = something_else add_node

AmirhosseinPoolad · 2025-11-20T21:08:27Z

libs/librrgraph/src/base/rr_graph_storage.cpp


 void t_rr_graph_storage::init_fan_in() {
    //Reset all fan-ins to zero
-    edges_read_ = true;


I agree that this method shouldn't set this flag, but I think I need a couple more eyes to look at this.

AmirhosseinPoolad · 2025-11-20T21:10:07Z

libs/librrgraph/src/base/rr_graph_storage.cpp

    for(const auto& edge_id : edge_dest_node_.keys()) {
        node_fan_in_[edge_dest_node_[edge_id]] += 1;
    }


Can you change this code to use .pairs() instead? Something like:

for (const auto& [edge_id, edge_destination_node] : edge_dest_node_.pairs())

Looks much nicer imo.

AmirhosseinPoolad · 2025-11-20T21:13:16Z

libs/librrgraph/src/base/rr_graph_storage.h

+    void set_node_ptc_nums(RRNodeId node, const std::string& ptc_str);
+    void add_node_tilable_track_num(RRNodeId node, size_t node_offset, short track_id);
+
+    void emplace_back_node_tilable_track_num();
+
+    bool node_contain_multiple_ptc(RRNodeId node) const {
+        if (node_tilable_track_nums_.empty()) {
+            return false;
+        } else {
+            return node_tilable_track_nums_[node].size() > 1;
+        }
+    }
+
+    std::string node_ptc_nums_to_string(RRNodeId node) const;
+
+    const std::vector<short>& node_tilable_track_nums(RRNodeId node) const {
+        return node_tilable_track_nums_[node];
+    }
+


Doxygen for all of these

AmirhosseinPoolad · 2025-11-20T21:13:59Z

libs/librrgraph/src/io/rr_graph_reader.cpp

+    if (graph_type == e_graph_type::UNIDIR_TILEABLE) {
+        rr_graph_builder->set_tileable(true);
+    }
+


Does this fix a bug?

AmirhosseinPoolad · 2025-11-20T21:54:34Z

libs/librrgraph/src/base/rr_graph_storage.cpp

+        for (size_t edge_index = 0; edge_index < edge_nodes.size(); ++edge_index) {
+            RREdgeId edge_id = RREdgeId(edge_index);


use all_edges() instead.

AmirhosseinPoolad · 2025-11-20T21:58:58Z

libs/librrgraph/src/base/rr_graph_storage.cpp

+    }
+
+    std::vector<RREdgeId> removed_edges;
+    auto adjust_edges = [&](vtr::vector<RREdgeId, RRNodeId>& edge_nodes) {


Add a comment explaining what this lambda does.

AmirhosseinPoolad · 2025-11-20T21:59:28Z

libs/librrgraph/src/base/rr_graph_storage.cpp

+                // Node exists in sorted_nodes, mark edge for removal
+                removed_edges.push_back(edge_id);
+            } else {
+                size_t node_offset = std::distance(sorted_nodes.begin(), node_it);


Not sure what's going on here. A comment would be nice.

AmirhosseinPoolad · 2025-11-20T22:01:00Z

libs/librrgraph/src/base/rr_graph_storage.h

+    /**
+     * @brief Removes a given list of RRNodes from the RR Graph
+     * This method should be called before partition_edges has been called.
+     * If init_fan_in has been called, you need to call it again after removing the nodes.
+     * @note This a very expensive method, so should be called only when necessary. It is better
+     * to not add nodes in the first place, instead of relying on this method to remove nodes.
+     *
+     * @param nodes list of RRNodes to be removed
+     */
+    void remove_nodes(const std::vector<RRNodeId>& nodes);
+


From what I can understand, this function is O(|V| + |E|logk) where k is the number of nodes you want to remove. You should add that to the doxygen.

AmirhosseinPoolad · 2025-11-20T22:01:25Z

libs/librrgraph/src/base/rr_graph_storage.cpp

+    adjust_edges(edge_src_node_);
+    adjust_edges(edge_dest_node_);


What about edge_switch_ and edge_remapped_?

AmirhosseinPoolad

Misclicked, didn't mean to approve. Sorry!

amin1377 added 13 commits November 17, 2025 11:08

[lib][rr_graph] initial impl for remove node

615bd5a

[libs][rr_graph] uniquify remove edges

511f89d

Merge branch 'rr_graph_interposer_cut' of https://github.com/verilog-…

112a6c0

…to-routing/vtr-verilog-to-routing into remove_node

[lib][rr_graph] add comments + shrink rr graph node data structures

5bbf3d7

fix a typo

dfc2a33

[lib][rr_graph] remove node_tilable_track_nums_ from rr graph builder

ec0a8e5

[lib][rr_graph] add node_tilable_track_nums_ to rr graph storage

7817922

[lib][rr_grpah] remove node_tileable_track_nums_ from rr_graph_view

54ff743

[lib][librrgraph] set tileable to true when reading rr graph from a f…

3da38b0

…ile if graph type is tileable

[libs][rrgraph] remove redundant function call

dda1dd5

[vpr] fix instances of rr graph view

de52b91

[lib][rr_graph] add remove_node header

4830963

[lib][rr_graph] erase node_tilable_track_nums_ when removing a node

3b9f552

github-actions bot added VPR VPR FPGA Placement & Routing Tool docs Documentation lang-cpp C/C++ code labels Nov 17, 2025

Merge branch 'rr_graph_interposer_cut' of https://github.com/verilog-…

5f49e17

…to-routing/vtr-verilog-to-routing into remove_node

amin1377 added 3 commits November 20, 2025 09:12

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

99e40a2

…rilog-to-routing into remove_node

Merge branch 'rr_graph_interposer_cut' of https://github.com/verilog-…

ec9b088

…to-routing/vtr-verilog-to-routing into remove_node

[lib][rr_graph][storage] fix bugs with remove_nodes

e5a0f13

amin1377 requested a review from AmirhosseinPoolad November 20, 2025 17:34

amin1377 changed the title ~~[WIP] Move node_tilable_track_nums_ to rr_graph_storage and add remove_node~~ Move node_tilable_track_nums_ to rr_graph_storage and add remove_node Nov 20, 2025

github-actions bot added the libarchfpga Library for handling FPGA Architecture descriptions label Nov 20, 2025

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

513115d

…rilog-to-routing into remove_node

AmirhosseinPoolad approved these changes Nov 20, 2025

View reviewed changes

AmirhosseinPoolad requested changes Nov 20, 2025

View reviewed changes

		for (size_t edge_index = 0; edge_index < edge_nodes.size(); ++edge_index) {
		RREdgeId edge_id = RREdgeId(edge_index);

Move node_tilable_track_nums_ to rr_graph_storage and add remove_node #3347

Are you sure you want to change the base?

Move node_tilable_track_nums_ to rr_graph_storage and add remove_node #3347

Conversation

amin1377 commented Nov 17, 2025

Uh oh!

AmirhosseinPoolad commented Nov 18, 2025

Uh oh!

amin1377 commented Nov 18, 2025

Uh oh!

AmirhosseinPoolad 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!

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!

AmirhosseinPoolad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants