Address PR comments

Author: AmirhosseinPoolad

libs/librrgraph/src/base/rr_graph_builder.h

@@ -351,7 +351,14 @@ class RRGraphBuilder {
     inline void alloc_and_load_edges(const t_rr_edge_info_set* rr_edges_to_create) {
         node_storage_.alloc_and_load_edges(rr_edges_to_create);
     }
-
+    
+    /** @brief Removes a given list of RREdgeIds for the RR Graph.
+     * This method does not preserve the order of edges. If you're
+     * calling it after partition_edges has been called, you need
+     * to call it again.
+     *
+     * @param rr_edges_to_remove list of RREdgeIds to be removed
+     */
     inline void remove_edges(std::vector<RREdgeId>& rr_edges_to_remove) {
         node_storage_.remove_edges(rr_edges_to_remove);
     }

libs/librrgraph/src/base/rr_graph_storage.cpp

@@ -62,21 +62,32 @@ 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) {
     size_t starting_edge_count = edge_dest_node_.size();
 
+    // Sort and make sure all edge indices are unique
     vtr::uniquify(rr_edges_to_remove);
+    VTR_ASSERT_SAFE(std::is_sorted(rr_edges_to_remove.begin(), rr_edges_to_remove.end()));
 
+    // Index of the last edge
     size_t edge_list_end = edge_dest_node_.size() - 1;
+
+    // Iterate backwards through the list of indices we want to remove.
     for (auto it = rr_edges_to_remove.rbegin(); it != rr_edges_to_remove.rend(); ++it) {
         RREdgeId erase_idx = *it;
 
+        // Copy what's at the end of the list to the index we wanted to remove
         edge_dest_node_[erase_idx] = edge_dest_node_[RREdgeId(edge_list_end)];
         edge_src_node_[erase_idx] = edge_src_node_[RREdgeId(edge_list_end)];
         edge_switch_[erase_idx] = edge_switch_[RREdgeId(edge_list_end)];
         edge_remapped_[erase_idx] = edge_remapped_[RREdgeId(edge_list_end)];
 
+        // At this point we have no copies of what was at erase_idx and two copies of
+        // what was at the end of the list. If we make the list one element shorter,
+        // we end up with a list that has removed the element at erase_idx.
         edge_list_end--;
 
     }
 
+    // We have a new index to the end of the list, call erase on the elements past that index
+    // to update the std::vector and shrink the actual data structures.
     edge_dest_node_.erase(edge_dest_node_.begin() + edge_list_end + 1, edge_dest_node_.end());
     edge_src_node_.erase(edge_src_node_.begin() + edge_list_end + 1, edge_src_node_.end());
     edge_switch_.erase(edge_switch_.begin() + edge_list_end + 1, edge_switch_.end());

libs/librrgraph/src/base/rr_graph_storage.h

@@ -400,7 +400,10 @@ class t_rr_graph_storage {
         return vtr::StrongIdRange<RREdgeId>(first_edge(id), last_edge(id));
     }
 
-
+    /** @brief Returns a range of all edges in the RR Graph.
+     * This method does not depend on the edges begin correctly
+     * sorted and can be used before partition_edges is called.
+     */
     inline vtr::StrongIdRange<RREdgeId> all_edges() const {
         return vtr::StrongIdRange<RREdgeId>(RREdgeId(0), RREdgeId(edge_src_node_.size()));
     }
@@ -781,6 +784,13 @@ class t_rr_graph_storage {
     /** @brief Adds a batch of edges.*/
     void alloc_and_load_edges(const t_rr_edge_info_set* rr_edges_to_create);
 
+    /** @brief Removes a given list of RREdgeIds for the RR Graph.
+     * This method does not preserve the order of edges. If you're
+     * calling it after partition_edges has been called, you need
+     * to call it again.
+     *
+     * @param rr_edges_to_remove list of RREdgeIds to be removed
+     */
     void remove_edges(std::vector<RREdgeId>& rr_edges_to_remove);
 
     /* Edge finalization methods */

libs/librrgraph/src/base/rr_graph_view.h

@@ -594,6 +594,10 @@ class RRGraphView {
         return vtr::make_range(edge_idx_iterator(0), edge_idx_iterator(num_edges(id)));
     }
 
+    /** @brief Returns a range of all edges in the RR Graph.
+     * This method does not depend on the edges begin correctly
+     * sorted and can be used before partition_edges is called.
+     */
     inline vtr::StrongIdRange<RREdgeId> all_edges() const {
         return node_storage_.all_edges();
     }

vpr/src/route/rr_graph_generation/interposer_cut.cpp

@@ -10,8 +10,8 @@
 #include "rr_graph_view.h"
 #include "rr_node_types.h"
 #include "rr_spatial_lookup.h"
+#include "vpr_error.h"
 #include "vtr_assert.h"
-#include "vtr_log.h"
 
 #include "interposer_cut.h"
 
@@ -43,23 +43,20 @@ static short node_xstart(const RRGraphView& rr_graph, RRNodeId node) {
     switch (rr_graph.node_direction(node)) {
         case Direction::DEC:
             return rr_graph.node_xhigh(node);
-            break;
 
         case Direction::INC:
             return rr_graph.node_xlow(node);
-            break;
 
         case Direction::NONE:
             VTR_ASSERT(rr_graph.node_xlow(node) == rr_graph.node_xhigh(node));
             return (rr_graph.node_xlow(node));
-            break;
 
         case Direction::BIDIR:
-            VTR_ASSERT_MSG(false, "Bidir node has no starting point");
+            VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "Bidir node has no starting point.");
             break;
 
         default:
-            VTR_ASSERT(false);
+            VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "Invalid RR node direction.");
             break;
     }
 }
@@ -76,23 +73,20 @@ static short node_ystart(const RRGraphView& rr_graph, RRNodeId node) {
     switch (rr_graph.node_direction(node)) {
         case Direction::DEC:
             return rr_graph.node_yhigh(node);
-            break;
 
         case Direction::INC:
             return rr_graph.node_ylow(node);
-            break;
 
         case Direction::NONE:
             VTR_ASSERT(rr_graph.node_ylow(node) == rr_graph.node_yhigh(node));
             return (rr_graph.node_ylow(node));
-            break;
 
         case Direction::BIDIR:
-            VTR_ASSERT_MSG(false, "Bidir node has no starting point");
+            VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "Bidir node has no starting point.");
             break;
 
         default:
-            VTR_ASSERT(false);
+            VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "Invalid RR node direction.");
             break;
     }
 }
@@ -106,10 +100,6 @@ std::vector<RREdgeId> mark_interposer_cut_edges_for_removal(const RRGraphView& r
         RRNodeId src_node = rr_graph.edge_src_node(edge_id);
         RRNodeId sink_node = rr_graph.edge_sink_node(edge_id);
 
-        if (src_node == RRNodeId(5866) && sink_node == RRNodeId(5604)) {
-            VTR_LOG("HI\n");
-        }
-
         // TODO: ignoring ChanZ nodes for now
         if (rr_graph.node_type(src_node) == e_rr_type::CHANZ || rr_graph.node_type(sink_node) == e_rr_type::CHANZ) {
             continue;
@@ -147,7 +137,10 @@ 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.
+  * @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) {
@@ -173,7 +166,10 @@ static void cut_chan_y_node(RRNodeId node, int x_low, int y_low, int x_high, int
 }
 
 /**
-  * @brief Update a CHANX 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.
+  * @brief Update a CHANX 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_x_node(RRNodeId node, int x_low, int y_low, int x_high, int y_high, int layer, int ptc_num, int cut_loc_x, Direction node_direction, RRGraphBuilder& rr_graph_builder, RRSpatialLookup& spatial_lookup) {
@@ -205,7 +201,6 @@ static void cut_chan_x_node(RRNodeId node, int x_low, int y_low, int x_high, int
  * @param cut_loc location of vertical interposer cut line
  * @param interposer_cut_type Type of the interposer cut line (Horizontal or vertical)
  * @param sg_node_indices Sorted list of scatter-gather node IDs. We do not want to cut these nodes as they're allowed to cross an interposer cut line.
- * @note This function is very similar to cut_chan_y_node. If you're modifying this you probably also want to modify that function too.
  */
 static void cut_channel_node(RRNodeId node, int cut_loc, e_interposer_cut_type interposer_cut_type, const RRGraphView& rr_graph, RRGraphBuilder& rr_graph_builder, RRSpatialLookup& spatial_lookup, const std::vector<std::pair<RRNodeId, int>>& sg_node_indices) {
     constexpr auto node_indice_compare = [](RRNodeId l, RRNodeId r) noexcept { return size_t(l) < size_t(r); };

vpr/src/route/rr_graph_generation/rr_node_indices.cpp

@@ -86,7 +86,7 @@ static void load_block_rr_indices(RRGraphBuilder& rr_graph_builder,
                                   const DeviceGrid& grid,
                                   int* index) {
     // Walk through the grid assigning indices to SOURCE/SINK IPIN/OPIN
-    for (const t_physical_tile_loc grid_loc : grid.all_locations()) {
+    for (const t_physical_tile_loc& grid_loc : grid.all_locations()) {
         //Process each block from its root location
         if (grid.is_root_location(grid_loc)) {
             t_physical_tile_type_ptr physical_type = grid.get_physical_type(grid_loc);
@@ -458,7 +458,7 @@ void alloc_and_load_intra_cluster_rr_node_indices(RRGraphBuilder& rr_graph_build
                                                   const vtr::vector<ClusterBlockId, std::unordered_set<int>>& pin_chains_num,
                                                   int* index) {
 
-    for (const t_physical_tile_loc grid_loc : grid.all_locations()) {
+    for (const t_physical_tile_loc& grid_loc : grid.all_locations()) {
 
         // Process each block from its root location
         if (grid.is_root_location(grid_loc)) {
@@ -498,7 +498,7 @@ bool verify_rr_node_indices(const DeviceGrid& grid,
                             const t_rr_graph_storage& rr_nodes,
                             bool is_flat) {
     std::unordered_map<RRNodeId, int> rr_node_counts;
-    for (t_physical_tile_loc tile_loc : grid.all_locations()) {
+    for (const t_physical_tile_loc& tile_loc : grid.all_locations()) {
         for (e_rr_type rr_type : RR_TYPES) {
             // Get the list of nodes at a specific location (x, y)
             std::vector<RRNodeId> nodes_from_lookup;