Address PR comments on RR Graph changes

AmirhosseinPoolad · AmirhosseinPoolad · commit 2d6de4feb997 · 2025-11-17T13:36:20.000-05:00
diff --git a/doc/src/arch/reference.rst b/doc/src/arch/reference.rst
@@ -2750,7 +2750,7 @@ The number of any additional wires or muxes created by scatter-gather specificat
     Overview of how scatter-gather patterns work. First, connections from a switchblock location are selected according to the specification.
     These selected connection are then muxed and passed through the scatter-gather node, which is typically a wire segment. The scatter-gather node then fans out or scatters in another switchblock location.
 
-.. note:: Scatter-Gather patterns are only supported for 3D architectures where the scatter-gather links are uni-directional and uni-directional 2D architectures. Bidirectional sg_links are not currently supported.
+.. note:: Scatter-Gather patterns are only supported for uni-directional 3D and uni-directional 2D architectures. Bidirectional sg_links are not currently supported.
 
 When instantiated, a scatter-gather pattern gathers connections from a switchblock and passes the connection through a multiplexer and the scatter-gather node which is typically a wire segment, then scatters or fans out somewhere else in the device. These patterns can be used to define 3D switchblocks. An example is shown below:
 
diff --git a/libs/librrgraph/src/base/rr_graph_storage.cpp b/libs/librrgraph/src/base/rr_graph_storage.cpp
@@ -60,6 +60,8 @@ void t_rr_graph_storage::alloc_and_load_edges(const t_rr_edge_info_set* rr_edges
 }
 
 void t_rr_graph_storage::remove_edges(std::vector<RREdgeId>& rr_edges_to_remove) {
+    VTR_ASSERT(!edges_read_);
+
     size_t starting_edge_count = edge_dest_node_.size();
 
     // Sort and make sure all edge indices are unique
diff --git a/libs/librrgraph/src/base/rr_graph_view.h b/libs/librrgraph/src/base/rr_graph_view.h
@@ -589,6 +589,8 @@ class RRGraphView {
      * for (t_edge_size edge : rr_graph.edges(node)) {
      *     // Do something with the edge
      * }
+     *
+     * @note Iterating on the range returned by this function will not give you an RREdgeId, but instead gives you the index among a node's outgoing edges
      */
     inline edge_idx_range edges(const RRNodeId& id) const {
         return vtr::make_range(edge_idx_iterator(0), edge_idx_iterator(num_edges(id)));
diff --git a/vpr/src/route/rr_graph_generation/interposer_cut.cpp b/vpr/src/route/rr_graph_generation/interposer_cut.cpp
@@ -137,12 +137,12 @@ std::vector<RREdgeId> mark_interposer_cut_edges_for_removal(const RRGraphView& r
 }
 
 /**
-  * @brief Update a CHANY node's bounding box in RRGraph and SpatialLookup entries.
-  * This function assumes that the channel node actually crosses the cut location and
-  * might not function correctly otherwise.
-  *
-  * This is a low level function, you should use cut_channel_node that wraps this up in a nicer API.
-  */
+ * @brief Update a CHANY node's bounding box in RRGraph and SpatialLookup entries.
+ * This function assumes that the channel node actually crosses the cut location and
+ * might not function correctly otherwise.
+ *
+ * This is a low level function, you should use cut_channel_node that wraps this up in a nicer API.
+ */
 static void cut_chan_y_node(RRNodeId node, int x_low, int y_low, int x_high, int y_high, int layer, int ptc_num, int cut_loc_y, Direction node_direction, RRGraphBuilder& rr_graph_builder, RRSpatialLookup& spatial_lookup) {
     if (node_direction == Direction::INC) {
         // Anything above cut_loc_y shouldn't exist
diff --git a/vpr/src/route/rr_graph_generation/interposer_cut.h b/vpr/src/route/rr_graph_generation/interposer_cut.h
@@ -20,4 +20,4 @@ std::vector<RREdgeId> mark_interposer_cut_edges_for_removal(const RRGraphView& r
  * @param rr_graph_builder RRGraphBuilder, to modify the RRGraph.
  * @param sg_node_indices list of scatter-gather node IDs. We do not want to cut these nodes as they're allowed to cross an interposer cut line.
  */
-void update_interposer_crossing_nodes_in_spatial_lookup_and_rr_graph_storage(const RRGraphView& rr_graph, const DeviceGrid& grid, RRGraphBuilder& rr_graph_builder, const std::vector<std::pair<RRNodeId, int>>& sg_node_indices);
+void update_interposer_crossing_nodes_in_spatial_lookup_and_rr_graph_storage(const RRGraphView& rr_graph, const DeviceGrid& grid, RRGraphBuilder& rr_graph_builder, const std::vector<std::pair<RRNodeId, int>>& sg_node_indices);
diff --git a/vpr/src/route/rr_graph_generation/rr_graph.cpp b/vpr/src/route/rr_graph_generation/rr_graph.cpp
@@ -1669,14 +1669,16 @@ static std::function<void(t_chan_width*)> alloc_and_load_rr_graph(RRGraphBuilder
             }
         }
     }
-
+    
+    // If there are any interposer cuts, remove the edges and shorten the wires that cross interposer cut lines.
     if (!grid.get_horizontal_interposer_cuts().empty() || !grid.get_vertical_interposer_cuts().empty()) {
         std::vector<RREdgeId> interposer_edges = mark_interposer_cut_edges_for_removal(rr_graph, grid);
         rr_graph_builder.remove_edges(interposer_edges);
 
         update_interposer_crossing_nodes_in_spatial_lookup_and_rr_graph_storage(rr_graph, grid, rr_graph_builder, sg_node_indices);
     }
-
+    
+    // Add 2D scatter-gather link edges (the nodes have already been created at this point). These links are mostly used for interposer-crossing connections, but could also be used for other things.
     add_and_connect_non_3d_sg_links(rr_graph_builder, sg_links, sg_node_indices, chan_details_x, chan_details_y, num_seg_types_x, rr_edges_to_create);
     uniquify_edges(rr_edges_to_create);
     alloc_and_load_edges(rr_graph_builder, rr_edges_to_create);
diff --git a/vpr/src/route/rr_graph_generation/rr_node_indices.cpp b/vpr/src/route/rr_graph_generation/rr_node_indices.cpp
@@ -518,12 +518,12 @@ bool verify_rr_node_indices(const DeviceGrid& grid,
                               describe_rr_node(rr_graph, grid, rr_indexed_data, inode, is_flat).c_str());
                 }
 
-                        if (tile_loc.layer_num < rr_graph.node_layer_low(inode) && tile_loc.layer_num > rr_graph.node_layer_high(inode)) {
-                            VPR_ERROR(VPR_ERROR_ROUTE, "RR node layer does not match between rr_nodes and rr_node_indices (%s/%s): %s",
-                                      rr_node_typename[rr_graph.node_type(inode)],
-                                      rr_node_typename[rr_type],
-                                      describe_rr_node(rr_graph, grid, rr_indexed_data, inode, is_flat).c_str());
-                        }
+                if (tile_loc.layer_num < rr_graph.node_layer_low(inode) && tile_loc.layer_num > rr_graph.node_layer_high(inode)) {
+                    VPR_ERROR(VPR_ERROR_ROUTE, "RR node layer does not match between rr_nodes and rr_node_indices (%s/%s): %s",
+                              rr_node_typename[rr_graph.node_type(inode)],
+                              rr_node_typename[rr_type],
+                              describe_rr_node(rr_graph, grid, rr_indexed_data, inode, is_flat).c_str());
+                }
 
                 if (rr_graph.node_type(inode) == e_rr_type::CHANX) {
                     VTR_ASSERT_MSG(rr_graph.node_ylow(inode) == rr_graph.node_yhigh(inode), "CHANX should be horizontal");
@@ -596,35 +596,35 @@ bool verify_rr_node_indices(const DeviceGrid& grid,
                 } else {
                     VTR_ASSERT(rr_graph.node_type(inode) == e_rr_type::IPIN || rr_graph.node_type(inode) == e_rr_type::OPIN);
                     /* As we allow a pin to be indexable on multiple sides,
-                             * This check code should be invalid
-                             * if (rr_node.xlow() != x) {
-                             *     VPR_ERROR(VPR_ERROR_ROUTE, "RR node xlow does not match between rr_nodes and rr_node_indices (%d/%d): %s",
-                             *               rr_node.xlow(),
-                             *               x,
-                             *               describe_rr_node(rr_graph, grid, rr_indexed_data, inode).c_str());
-                             * }
-                             *
-                             * if (rr_node.ylow() != y) {
-                             *     VPR_ERROR(VPR_ERROR_ROUTE, "RR node ylow does not match between rr_nodes and rr_node_indices (%d/%d): %s",
-                             *               rr_node.ylow(),
-                             *               y,
-                             *               describe_rr_node(rr_graph, grid, rr_indexed_data, inode).c_str());
-                             * }
-                             */
+                     * This check code should be invalid
+                     * if (rr_node.xlow() != x) {
+                     *     VPR_ERROR(VPR_ERROR_ROUTE, "RR node xlow does not match between rr_nodes and rr_node_indices (%d/%d): %s",
+                     *               rr_node.xlow(),
+                     *               x,
+                     *               describe_rr_node(rr_graph, grid, rr_indexed_data, inode).c_str());
+                     * }
+                     *
+                     * if (rr_node.ylow() != y) {
+                     *     VPR_ERROR(VPR_ERROR_ROUTE, "RR node ylow does not match between rr_nodes and rr_node_indices (%d/%d): %s",
+                     *               rr_node.ylow(),
+                     *               y,
+                     *               describe_rr_node(rr_graph, grid, rr_indexed_data, inode).c_str());
+                     * }
+                     */
                 }
 
                 if (rr_type == e_rr_type::IPIN || rr_type == e_rr_type::OPIN) {
                     /* As we allow a pin to be indexable on multiple sides,
-                             * This check code should be invalid
-                             * if (rr_node.side() != side) {
-                             *     VPR_ERROR(VPR_ERROR_ROUTE, "RR node xlow does not match between rr_nodes and rr_node_indices (%s/%s): %s",
-                             *               TOTAL_2D_SIDE_STRINGS[rr_node.side()],
-                             *               TOTAL_2D_SIDE_STRINGS[side],
-                             *               describe_rr_node(rr_graph, grid, rr_indexed_data, inode).c_str());
-                             * } else {
-                             *     VTR_ASSERT(rr_node.side() == side);
-                             * }
-                             */
+                     * This check code should be invalid
+                     * if (rr_node.side() != side) {
+                     *     VPR_ERROR(VPR_ERROR_ROUTE, "RR node xlow does not match between rr_nodes and rr_node_indices (%s/%s): %s",
+                     *               TOTAL_2D_SIDE_STRINGS[rr_node.side()],
+                     *               TOTAL_2D_SIDE_STRINGS[side],
+                     *               describe_rr_node(rr_graph, grid, rr_indexed_data, inode).c_str());
+                     * } else {
+                     *     VTR_ASSERT(rr_node.side() == side);
+                     * }
+                     */
                 }
             }
         }

Original file line number	Diff line number	Diff line change
`@@ -60,6 +60,8 @@ void t_rr_graph_storage::alloc_and_load_edges(const t_rr_edge_info_set* rr_edges`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`void t_rr_graph_storage::remove_edges(std::vector<RREdgeId>& rr_edges_to_remove) {`
	`63`	`+ VTR_ASSERT(!edges_read_);`
	`64`	`+`
`63`	`65`	`size_t starting_edge_count = edge_dest_node_.size();`
`64`	`66`
`65`	`67`	`// Sort and make sure all edge indices are unique`
Original file line number	Diff line number	Diff line change
`@@ -1669,14 +1669,16 @@ static std::function<void(t_chan_width*)> alloc_and_load_rr_graph(RRGraphBuilder`
`1669`	`1669`	`}`
`1670`	`1670`	`}`
`1671`	`1671`	`}`
`1672`		`-`
	`1672`	`+`
	`1673`	`+ // If there are any interposer cuts, remove the edges and shorten the wires that cross interposer cut lines.`
`1673`	`1674`	`if (!grid.get_horizontal_interposer_cuts().empty() \|\| !grid.get_vertical_interposer_cuts().empty()) {`
`1674`	`1675`	`std::vector<RREdgeId> interposer_edges = mark_interposer_cut_edges_for_removal(rr_graph, grid);`
`1675`	`1676`	`rr_graph_builder.remove_edges(interposer_edges);`
`1676`	`1677`
`1677`	`1678`	`update_interposer_crossing_nodes_in_spatial_lookup_and_rr_graph_storage(rr_graph, grid, rr_graph_builder, sg_node_indices);`
`1678`	`1679`	`}`
`1679`		`-`
	`1680`	`+`
	`1681`	`+ // Add 2D scatter-gather link edges (the nodes have already been created at this point). These links are mostly used for interposer-crossing connections, but could also be used for other things.`
`1680`	`1682`	`add_and_connect_non_3d_sg_links(rr_graph_builder, sg_links, sg_node_indices, chan_details_x, chan_details_y, num_seg_types_x, rr_edges_to_create);`
`1681`	`1683`	`uniquify_edges(rr_edges_to_create);`
`1682`	`1684`	`alloc_and_load_edges(rr_graph_builder, rr_edges_to_create);`