Comment edits

Author: Nathan Shreve

libs/librrgraph/src/base/check_rr_graph.cpp

@@ -409,7 +409,7 @@ void check_rr_node(const RRGraphView& rr_graph,
             vtr::Rect<int> tile_bb = grid.get_tile_bb({xlow, ylow, layer_num});
             if (xlow < tile_bb.xmin() || ylow < tile_bb.ymin() || xhigh > tile_bb.xmax() || yhigh > tile_bb.ymax()) {
                 VPR_FATAL_ERROR(VPR_ERROR_ROUTE,
-                                "in check_rr_node: node %d (type %d) has endpoints (%d,%d) and (%d,%d)\n", inode, rr_type, xlow, ylow, xhigh, yhigh);
+                                "in check_rr_node: node %d (type %d) has endpoints (%d,%d) and (%d,%d), which is outside the bounds of the grid tile containing it.\n", inode, rr_type, xlow, ylow, xhigh, yhigh);
             }
             break;
         }

libs/librrgraph/src/base/rr_graph_utils.cpp

@@ -6,9 +6,23 @@
 #include "rr_graph_obj.h"
 #include "rr_graph_builder.h"
 
-// Add "cluster-edge" IPINs to sink_ipins
-static void walk_cluster_recursive(const RRGraphView& rr_graph,
-                                   const vtr::vector<RRNodeId, std::vector<RREdgeId>>& fanins,
+/*
+ * @brief Walk backwards from origin SINK, and insert all cluster-edge IPINs to which origin is connected to sink_ipins
+ *
+ * @param rr_graph
+ * @param fanins A vector where, at each node index, is a vector of edges which are fanins of that node
+ * @param sink_ipins The set in which cluster-edge IPINs will be collected; should be empty
+ * @param curr The current node in recursion; originally, should be the same as origin
+ * @param origin The SINK whose cluster-edge IPINs are to be collected
+ */
+static void rr_walk_cluster_recursive(const RRGraphView& rr_graph,
+                                      const vtr::vector<RRNodeId, std::vector<RREdgeId>>& fanins,
+                                      std::unordered_set<RRNodeId>& sink_ipins,
+                                      const RRNodeId curr,
+                                      const RRNodeId origin);
+
+static void rr_walk_cluster_recursive(const RRGraphView& rr_graph,
+                                      const vtr::vector<RRNodeId, std::vector<RREdgeId>>& fanins,
                                    std::unordered_set<RRNodeId>& sink_ipins,
                                    const RRNodeId curr,
                                    const RRNodeId origin) {
@@ -37,7 +51,7 @@ static void walk_cluster_recursive(const RRGraphView& rr_graph,
         }
 
         // If the parent node is intra-cluster, keep going "backward"
-        walk_cluster_recursive(rr_graph, fanins, sink_ipins, parent, origin);
+        rr_walk_cluster_recursive(rr_graph, fanins, sink_ipins, parent, origin);
     }
 }
 
@@ -169,7 +183,7 @@ void rr_set_sink_locs(const RRGraphView& rr_graph, RRGraphBuilder& rr_graph_buil
         } else { /* We have not seen this tile type/ptc combo before */
             // The IPINs of the current SINK node
             std::unordered_set<RRNodeId> sink_ipins = {};
-            walk_cluster_recursive(rr_graph, node_fanins, sink_ipins, node_id, node_id);
+            rr_walk_cluster_recursive(rr_graph, node_fanins, sink_ipins, node_id, node_id);
 
             /* Set SINK locations as average of collected IPINs */
 

libs/librrgraph/src/base/rr_graph_utils.h

@@ -44,16 +44,22 @@ struct t_cluster_pin_chain {
 };
 
 /**
- * @brief Get node-to-node switches in a RRGraph
+ * @brief Get switches in a RRGraph starting at from_node and ending at to_node.
  *
- * @return A vector of switch ids
+ * @details
+ * Uses RRGraphView::find_edges(), then converts these edges to switch IDs.
+ *
+ * @return A vector of switch IDs
  */
 std::vector<RRSwitchId> find_rr_graph_switches(const RRGraph& rr_graph,
                                                const RRNodeId& from_node,
                                                const RRNodeId& to_node);
 
 /**
- * @brief This function generates and returns a vector indexed by RRNodeId containing a list of fan-in edges for each node.
+ * @brief This function generates and returns a vector indexed by RRNodeId containing a vector of fan-in edges for each node.
+ *
+ * @note
+ * This function is CPU expensive; complexity O(E) where E is the number of edges in rr_graph
  */
 vtr::vector<RRNodeId, std::vector<RREdgeId>> get_fan_in_list(const RRGraphView& rr_graph);
 
@@ -62,17 +68,7 @@ vtr::vector<RRNodeId, std::vector<RREdgeId>> get_fan_in_list(const RRGraphView&
  *
  * @details
  * build_rr_graph() sets the location of SINK nodes to span the entire tile they are in. This function sets the location
- * of SINK nodes to be the average coordinate of the IPINs on their cluster block to which they are connected
- *
- * @warning
- * 1. Do not call this function if the RR graph will be written out.<BR>
- * 2. If using flat routing, this function must be called before building the intra-cluster RR graph.
- *
- * @todo
- * Either when writing out the RR graph after this function has been called, or reading in an RR graph produced in VPR
- * after this function was called, an error occurs (many IPINs have no fanins). The reason for this error has not been
- * determined. However, this is quite a big issue, as choosing to write out the RR graph now significantly increases
- * runtime!
+ * of SINK nodes to be the average coordinate of the "cluster-edge" IPINs to which they are connected
  */
 void rr_set_sink_locs(const RRGraphView& rr_graph, RRGraphBuilder& rr_graph_builder, const DeviceGrid& grid);
 
@@ -83,7 +79,7 @@ void rr_set_sink_locs(const RRGraphView& rr_graph, RRGraphBuilder& rr_graph_buil
 int seg_index_of_cblock(const RRGraphView& rr_graph, t_rr_type from_rr_type, int to_node);
 
 /**
- * @breif Returns the segment number (distance along the channel) of the switch box from from_node (CHANX or CHANY) to
+ * @brief Returns the segment number (distance along the channel) of the switch box from from_node (CHANX or CHANY) to
  * to_node (CHANX or CHANY).
  *
  * @details
@@ -103,14 +99,4 @@ int seg_index_of_sblock(const RRGraphView& rr_graph, int from_node, int to_node)
  * @return limited_to_opin
  */
 bool inter_layer_connections_limited_to_opin(const RRGraphView& rr_graph);
-
-/**
- * @brief This function returns a rectangle which represents the bounding box of the tile at the given location.
- *
- * @param tile_loc
- * @param rr_graph
- * @param grid
- *
- * @return tile_bounding_box
- */
 #endif

libs/librrgraph/src/base/rr_spatial_lookup.cpp

@@ -295,6 +295,7 @@ bool RRSpatialLookup::remove_node(RRNodeId node,
     VTR_ASSERT_SAFE(ptc >= 0);
     VTR_ASSERT_SAFE(side != NUM_SIDES);
 
+    // Check if the node given is in the spatial lookup at the given indices
     if (type >= rr_node_indices_.size()) return false;
     if ((size_t)layer >= rr_node_indices_[type].dim_size(0)) return false;
     if ((size_t)x >= rr_node_indices_[type].dim_size(1)) return false;
@@ -303,6 +304,8 @@ bool RRSpatialLookup::remove_node(RRNodeId node,
     if ((size_t)ptc >= rr_node_indices_[type][layer][x][y][side].size()) return false;
     if (rr_node_indices_[type][layer][x][y][side][ptc] != int(size_t(node))) return false;
 
+    // The node was in the spatial lookup; remove it. -1 corresponds to an invalid node id,
+    // and so is treated as absent in the spatial lookup
     rr_node_indices_[type][layer][x][y][side][ptc] = -1;
     return true;
 }

libs/librrgraph/src/base/rr_spatial_lookup.h

@@ -42,7 +42,7 @@ class RRSpatialLookup {
      * @brief Returns the index of the specified routing resource node.  
      *
      *   @param layer specified which FPGA die the node is located at (e.g. multi-die(3D) FPGA)
-     *   @param (x, y) are the grid location within the FPGA
+     *   @param (x, y) is the grid location within the FPGA
      *   @param rr_type specifies the type of resource,
      *   @param ptc gives a unique number of resources of that type (e.g. CHANX) at that (layer,x,y).
      *
@@ -80,7 +80,7 @@ class RRSpatialLookup {
      *
      *   @param layer specifies which FPGA die the node is located at (e.g. multi-die(3D) FPGA)
      *   @param (xlow, ylow) is the lower left corner of the grid location range to search within the FPGA
-     *   @param (xlow, ylow) is the top right corner of the grid location range to search within the FPGA
+     *   @param (xhigh, yhigh) is the top right corner of the grid location range to search within the FPGA
      *   @param rr_type specifies the type of resource,
      *   @param ptc gives a unique number of resources of that type (e.g. CHANX) at that (layer,x,y).
      *
@@ -102,7 +102,7 @@ class RRSpatialLookup {
      * @brief Returns the indices of the specified routing resource nodes, representing routing tracks in a channel.  
      *
      *   @param layer specified which FPGA die the node is located at (e.g. multi-die(3D) FPGA)
-     *   @param (x, y) are the coordinate of the routing channel within the FPGA
+     *   @param (x, y) is the coordinate of the routing channel within the FPGA
      *   @param rr_type specifies the type of routing channel, either x-direction or y-direction
      *
      * @note 
@@ -151,18 +151,19 @@ class RRSpatialLookup {
                        e_side side = SIDES[0]);
 
     /**
-     * @brief Register a node in the fast look-up 
+     * @brief Register a node in the fast spatial lookup
      *
      * @note You must have a valid node id to register the node in the lookup
      *
      *   @param layer specified which FPGA die the node is located at (e.g. multi-die(3D) FPGA)
-     *   @param (x, y) are the coordinate of the node to be indexable in the fast look-up
+     *   @param (x, y) is the coordinate of the node to be indexable in the fast spatial lookup
      *   @param type is the type of a node
-     *   @param ptc is a feature number of a node, which can be
-     *     - the class number of a common SINK/SOURCE node of grid, 
-     *     - pin index in a tile when type is OPIN/IPIN
+     *   @param ptc is a feature number of a node, which can be<BR>
+     *     - the class number of a common SINK/SOURCE node of grid,<BR>
+     *     - pin index in a tile when type is OPIN/IPIN<BR>
      *     - track index in a routing channel when type is CHANX/CHANY
-     *   @param side is the side of node on the tile, applicable to OPIN/IPIN 
+     *   @param side is the side of node on the tile, applicable to OPIN/IPIN; it is ignored for
+     * other types, and hence has a default set
      *
      * @note a node added with this call will not create a node in the rr_graph_storage node list
      * You MUST add the node in the rr_graph_storage so that the node is valid  
@@ -183,16 +184,17 @@ class RRSpatialLookup {
                   e_side side = SIDES[0]);
 
     /**
-     * @brief Remove a node in the fast lookup.
+     * @brief Remove a node in the fast spatial lookup.
      *
      * @param layer specified which FPGA die the node is located at (e.g. multi-die(3D) FPGA)
-     * @param (x, y) are the coordinate of the node
+     * @param (x, y) is the coordinate of the node
      * @param type is the type of a node
-     * @param ptc is a feature number of a node, which can be
-     *     - the class number of a common SINK/SOURCE node of grid,
-     *     - pin index in a tile when type is OPIN/IPIN
+     * @param ptc is a feature number of a node, which can be<BR>
+     *     - the class number of a common SINK/SOURCE node of grid,<BR>
+     *     - pin index in a tile when type is OPIN/IPIN<BR>
      *     - track index in a routing channel when type is CHANX/CHANY
-     * @param side is the side of node on the tile, applicable to OPIN/IPIN
+     * @param side is the side of node on the tile, applicable to OPIN/IPIN; it is ignored for
+     * other types, and hence has a default set
      *
      * @return success Whether the node was removed successfully. If the function returns false,
      * the node was not in the lookup at the indices provided.
@@ -209,7 +211,7 @@ class RRSpatialLookup {
      * @brief Mirror the last dimension of a look-up, i.e., a list of nodes, from a source coordinate to
      * a destination coordinate.
      *
-     * This function is mostly need by SOURCE nodes which are indexable in multiple locations.
+     * This function is mostly needed by SOURCE nodes which are indexable in multiple locations.
      * Considering a bounding box (layer, x, y)->(layer, x + width, y + height) of a multi-height and multi-width grid,
      * SOURCE nodes are indexable in any location inside the boundary.
      *

libs/libvtrutil/src/vtr_geometry.h

@@ -208,10 +208,10 @@ class Rect {
     ///@brief set ymax to a point
     void set_ymax(T ymax_val);
 
-    /// @brief += operator
+    /// @brief += operator; translates all corners of the rectangle by rhs
     Rect<T>& operator+=(const Point<T>& rhs);
 
-    /// @brief -= operator
+    /// @brief -= operator; translates all corners of the rectangle by -rhs
     Rect<T>& operator-=(const Point<T>& rhs);
 
     ///@brief Equivalent to `*this = bounding_box(*this, other)`

vpr/src/base/vpr_types.h

@@ -123,7 +123,7 @@ enum class e_router_lookahead {
     MAP,
     ///@brief Similar to MAP, but use a sparse sampling of the chip
     COMPRESSED_MAP,
-    ///@breif Lookahead with a more extensive node sampling method
+    ///@brief Lookahead with a more extensive node sampling method
     EXTENDED_MAP,
     ///@brief A no-operation lookahead which always returns zero
     NO_OP

vpr/src/route/check_route.cpp

@@ -301,7 +301,8 @@ static bool check_adjacent(RRNodeId from_node, RRNodeId to_node, bool is_flat) {
     to_yhigh = rr_graph.node_yhigh(to_rr);
     to_ptc = rr_graph.node_ptc_num(to_rr);
 
-    // If to_node is SINK, use its tile coordinates
+    // If to_node is a SINK, it could be anywhere within its containing device grid tile, and it is reasonable for
+    // any input pins or within-cluster pins to reach it. Hence, treat its size as that of its containing tile.
     if (to_type == SINK) {
         vtr::Rect<int> tile_bb = device_ctx.grid.get_tile_bb({to_xlow, to_ylow, to_layer});