applied PR comments

Author: soheilshahrouz

vpr/src/base/read_options.cpp

@@ -2813,28 +2813,39 @@ argparse::ArgumentParser create_arg_parser(std::string prog_name, t_options& arg
     noc_grp.add_argument<double>(args.noc_placement_weighting, "--noc_placement_weighting")
         .help(
             "Controls the importance of the NoC placement parameters relative to timing and wirelength of the design."
-            "This value can be >=0, where 0 would mean the placement is based solely on timing and wirelength, a value of 1 would mean noc placement is considered equal to timing and wirelength and a value greater than 1 would mean the placement is increasingly dominated by NoC parameters.")
+            "This value can be >=0, where 0 would mean the placement is based solely on timing and wirelength."
+            "A value of 1 would mean noc placement is considered equal to timing and wirelength"
+            "A value greater than 1 would mean the placement is increasingly dominated by NoC parameters.")
         .default_value("5.0")
         .show_in(argparse::ShowIn::HELP_ONLY);
 
     noc_grp.add_argument<double>(args.noc_latency_constraints_weighting, "--noc_latency_constraints_weighting")
         .help(
-            "Controls the importance of meeting all the NoC traffic flow latency constraints."
-            "This value can be >=0, where 0 would mean the latency constraints have no relevance to placement, a value of 1 would mean the latency constraints are weighted equally to the sum of other placement cost components and a value greater than 1 would mean the placement is increasingly dominated by meeting the latency constraints of the traffic flows.")
+            "Controls the importance of meeting all the NoC traffic flow latency constraints.\n"
+            "This value can be >=0, where 0 would mean the latency constraints have no relevance to placement.\n"
+            "Other positive numbers specify the importance of meeting latency constraints to other NoC-related cost terms.\n"
+            "Weighting factors for NoC-related cost terms are normalized internally. Therefore, their absolute values are not important, and"
+            "only their relative ratios determine the importance of each cost term.")
         .default_value("0.6")
         .show_in(argparse::ShowIn::HELP_ONLY);
 
     noc_grp.add_argument<double>(args.noc_latency_weighting, "--noc_latency_weighting")
         .help(
-            "Controls the importance of reducing the latencies of the NoC traffic flows."
-            "This value can be >=0, where 0 would mean the latencies have no relevance to placement, a value of 1 would mean the latencies  are weighted equally to the sum of other placement cost components and a value greater than 1 would mean the placement is increasingly dominated by reducing the latencies of the traffic flows.")
+            "Controls the importance of reducing the latencies of the NoC traffic flows.\n"
+            "This value can be >=0, where 0 would mean the latencies have no relevance to placement.\n"
+            "Other positive numbers specify the importance of minimizing aggregate latency to other NoC-related cost terms.\n"
+            "Weighting factors for NoC-related cost terms are normalized internally. Therefore, their absolute values are not important, and"
+            "only their relative ratios determine the importance of each cost term.")
         .default_value("0.02")
         .show_in(argparse::ShowIn::HELP_ONLY);
 
     noc_grp.add_argument<double>(args.noc_congestion_weighting, "--noc_congestion_weighting")
         .help(
-            "Controls the importance of reducing the congestion of the NoC links."
-            "This value can be >=0, where 0 would mean the congestion has no relevance to placement, a value of 1 would mean the congestion is weighted equally to the sum of other placement cost components and a value greater than 1 would mean the placement is increasingly dominated by reducing the link congestions.")
+            "Controls the importance of reducing the congestion of the NoC links.\n"
+            "This value can be >=0, where 0 would mean the congestion has no relevance to placement.\n"
+            "Other positive numbers specify the importance of minimizing congestion to other NoC-related cost terms.\n"
+            "Weighting factors for NoC-related cost terms are normalized internally. Therefore, their absolute values are not important, and"
+            "only their relative ratios determine the importance of each cost term.")
         .default_value("0.00")
         .show_in(argparse::ShowIn::HELP_ONLY);
 

vpr/src/base/vpr_types.h

@@ -1498,11 +1498,11 @@ struct t_noc_opts {
     std::string noc_flows_file;               ///<name of the file that contains all the traffic flow information to be sent over the NoC in this design
     std::string noc_routing_algorithm;        ///<controls the routing algorithm used to route packets within the NoC
     double noc_placement_weighting;           ///<controls the significance of the NoC placement cost relative to the total placement cost range:[0-inf)
-    double noc_aggregate_bandwidth_weighting;
-    double noc_latency_constraints_weighting; ///<controls the significance of meeting the traffic flow contraints range:[0-inf)
+    double noc_aggregate_bandwidth_weighting; ///<controls the significance of aggregate used bandwidth relative to other NoC placement costs:[0:-inf)
+    double noc_latency_constraints_weighting; ///<controls the significance of meeting the traffic flow constraints range:[0-inf)
     double noc_latency_weighting;             ///<controls the significance of the traffic flow latencies relative to the other NoC placement costs range:[0-inf)
     double noc_congestion_weighting;          ///<controls the significance of the link congestions relative to the other NoC placement costs range:[0-inf)
-    int noc_swap_percentage;                  ///<controls the number of NoC router block swap attemps relative to the total number of swaps attempted by the placer range:[0-100]
+    int noc_swap_percentage;                  ///<controls the number of NoC router block swap attempts relative to the total number of swaps attempted by the placer range:[0-100]
     std::string noc_placement_file_name;      ///<is the name of the output file that contains the NoC placement information
 };
 

vpr/src/noc/noc_storage.h

@@ -30,7 +30,7 @@
  * in the NoC. They can be thought of as edges in a graph. Links
  * have a source router where they exit from and sink router where
  * they enter. It is important to note that the links are not
- * bi-directional, the legal way to traverse a link is from the
+ * bi-directional; the legal way to traverse a link is from the
  * source router of the link to the sink router.
  * 
  */
@@ -271,8 +271,10 @@ class NocStorage {
 
     /**
      * @brief Given source and sink router identifiers, this function
-     * finds a link connecting these routers and returns it identifier.
+     * finds a link connecting these routers and returns its identifier.
      * If such a link does not exist, an invalid id is returned.
+     * The function is not optimized for performance as it has a complexity
+     * of O(N_links).
      *
      * @param src_router The unique router identifier for the source router.
      * @param dst_router The unique router identifier for the destination router.

vpr/src/place/noc_place_utils.cpp

@@ -86,9 +86,12 @@ void reinitialize_noc_routing(t_placer_costs& costs) {
 
 void find_affected_noc_routers_and_update_noc_costs(const t_pl_blocks_to_be_moved& blocks_affected,
                                                     NocCostTerms& delta_c) {
+    /* For speed, delta_c is passed by reference instead of being returned.
+     * We expect delta cost terms to be zero to ensure correctness.
+     */
     VTR_ASSERT_SAFE(delta_c.aggregate_bandwidth == 0.);
     VTR_ASSERT_SAFE(delta_c.latency == 0.);
-    VTR_ASSERT(delta_c.latency_overrun == 0.);
+    VTR_ASSERT_SAFE(delta_c.latency_overrun == 0.);
     VTR_ASSERT_SAFE(delta_c.congestion == 0.);
     auto& noc_ctx = g_vpr_ctx.mutable_noc();
 
@@ -230,7 +233,7 @@ void re_route_associated_traffic_flows(ClusterBlockId moved_block_router_id,
             // first check to see whether we have already re-routed the current traffic flow and only re-route it if we haven't already.
             if (updated_traffic_flows.find(traffic_flow_id) == updated_traffic_flows.end()) {
                 // get all links for this flow route before it is rerouted
-                // The returned const std::vector<NocLinkId>& is copied so that we can modify (sort) it
+                // The returned const std::vector<NocLinkId>& is copied so that we can modify (sort) it in find_affected_links_by_flow_reroute()
                 std::vector<NocLinkId> prev_traffic_flow_links = noc_traffic_flows_storage.get_traffic_flow_route(traffic_flow_id);
 
                 // now update the current traffic flow by re-routing it based on the new locations of its src and destination routers
@@ -603,9 +606,16 @@ double calculate_noc_cost(const NocCostTerms& cost_terms,
     double cost = 0.0;
 
     /* NoC's contribution to the placement cost is a weighted sum over:
-     * 1) Traffic flow latency costs
-     * 2) Traffic flow aggregate bandwidth costs
-     * 3) Link congestion costs
+     * 1) Traffic flow aggregate bandwidth cost
+     * 2) Traffic flow latency cost
+     * 3) Traffic flow latency overrun cost
+     * 4) Link congestion cost
+     *
+     * Since NoC-related cost terms have different scales, they are
+     * rescaled by multiplying each cost term with its corresponding
+     * normalization factor. Then, a weighted sum over normalized cost terms
+     * is computed. Weighting factors determine the contribution of each
+     * normalized term to the sum.
      */
     cost = noc_opts.noc_placement_weighting * (
                cost_terms.aggregate_bandwidth * norm_factors.aggregate_bandwidth * noc_opts.noc_aggregate_bandwidth_weighting +

vpr/src/place/noc_place_utils.h

@@ -35,6 +35,8 @@ constexpr double INVALID_NOC_COST_TERM = -1.0;
  * @brief Each traffic flow cost consists of two components:
  *        1) traffic flow aggregate bandwidth (sum over all used links of the traffic flow bandwidth)
  *        2) traffic flow latency (currently unloaded/best-case latency of the flow)
+ *        3) traffic flow latency overrun (how much the latency is higher than the
+ *        latency constraint for a traffic flow.
  *        NoC placement code will keep an array-of-struct to easily access each
  *        traffic flow cost.
  */
@@ -378,6 +380,9 @@ double calculate_traffic_flow_aggregate_bandwidth_cost(const std::vector<NocLink
  * latencies.
  * @param traffic_flow_info Contains the traffic flow priority.
  * @return The computed latency cost terms for the given traffic flow.
+ * The first element is the total latency experience by the traffic flow.
+ * The second one specifies how much the experienced latency exceeds the
+ * latency constraint set for this traffic flow.
  */
 std::pair<double, double> calculate_traffic_flow_latency_cost(const std::vector<NocLinkId>& traffic_flow_route,
                                                               const NocStorage& noc_model,
@@ -394,6 +399,15 @@ std::pair<double, double> calculate_traffic_flow_latency_cost(const std::vector<
  */
 double calculate_link_congestion_cost(const NocLink& link);
 
+/**
+ * @brief The user passes weighting factors for aggregate latency
+ * and latency overrun terms. The weighting factor for aggregate
+ * bandwidth is computed by subtracting two user-provided weighting
+ * factor from 1. The computed aggregate bandwidth weighting factor
+ * is stored in noc_opts argument.
+ *
+ * @param noc_opts Contains weighting factors.
+ */
 void normalize_noc_cost_weighting_factor(t_noc_opts& noc_opts);
 
 /**
@@ -424,6 +438,8 @@ int get_number_of_traffic_flows_with_latency_cons_met(void);
 
 /**
  * @brief Goes through all NoC links and counts the congested ones.
+ * A congested NoC link is a link whose used bandwidth exceeds its
+ * bandwidth capacity.
  *
  * @return The total number of congested NoC links.
  */

vpr/src/place/place_util.cpp

@@ -431,7 +431,7 @@ void alloc_and_load_legal_placement_locations(std::vector<std::vector<std::vecto
                             continue;
                         }
                         // If this is the anchor position of a block, add it to the legal_pos.
-                        // Otherwise don't, so large blocks aren't added multiple times.
+                        // Otherwise, don't, so large blocks aren't added multiple times.
                         if (device_ctx.grid.get_width_offset({i, j, layer_num}) == 0 && device_ctx.grid.get_height_offset({i, j, layer_num}) == 0) {
                             int itype = tile->index;
                             int isub_tile = sub_tile.index;

vpr/src/place/place_util.h

@@ -18,12 +18,18 @@ class t_placer_costs;
 
 /**
  * @brief Data structure that stores different cost terms for NoC placement.
+ * This data structure can also be used to store normalization and weighting
+ * factors for NoC-related cost terms.
  *
- *   @param aggregate_bandwidth The total used bandwidth used in the NoC.
- *   @param latency A weighted average between aggregate latency and
- *   latency overruns.
- *   @param congestion The sum of congestion divided by available bandwidth
- *   over all NoC links.
+ *   @param aggregate_bandwidth The aggregate NoC bandwidth cost. This is
+ *   computed by summing all used link bandwidths.
+ *   @param latency The NoC latency cost, calculated as the sum of latencies
+ *   experienced by each traffic flow.
+ *   @param latency_overrun Sum of latency overrun for traffic flows that have
+ *   a latency constraint.
+ *   @param congestion The NoC congestion cost, i.e. how over-utilized
+ *   NoC links are. This is computed by dividing over-utilized bandwidth
+ *   by link bandwidth, and summing all computed ratios.
  */
 struct NocCostTerms {
   public:
@@ -61,22 +67,14 @@ struct NocCostTerms {
  *   @param timing_cost_norm The normalization factor for the timing cost, which
  *              is upper-bounded by the value of MAX_INV_TIMING_COST.
  *
- *   @param noc_aggregate_bandwidth_cost The aggregate NoC bandwidth cost
- *   @param noc_aggregate_bandwidth_cost_norm The normalization factor for
- *   the aggregate bandwidth cost
- *   @param noc_latency_cost The NoC latency cost,
- *   calculated as the sum of latencies experienced by each traffic flow
- *   @param noc_latency_cost_norm The normalization factor for the latency cost
- *   @param noc_congestion_cost The NoC congestion cost, i.e. how over-utilized
- *   NoC links are
- *   @param noc_congestion_cost_norm The normalization factor for the NoC
- *   congestion cost
+ *   @param noc_cost_terms NoC-related cost terms
+ *   @param noc_cost_norm_factors Normalization factors for NoC-related cost terms.
  *
  *   @param MAX_INV_TIMING_COST Stops inverse timing cost from going to infinity
  *              with very lax timing constraints, which avoids multiplying by a
  *              gigantic timing_cost_norm when auto-normalizing. The exact value
  *              of this cost has relatively little impact, but should be large
- *              enough to not affect the timing costs computatation for normal 
+ *              enough to not affect the timing costs computation for normal
  *              constraints.
  *
  *   @param place_algorithm Determines how the member values are updated upon
@@ -94,7 +92,7 @@ class t_placer_costs {
     NocCostTerms noc_cost_norm_factors;
 
   public: //Constructor
-    t_placer_costs(t_place_algorithm algo)
+    explicit t_placer_costs(t_place_algorithm algo)
         : place_algorithm(algo) {}
     t_placer_costs() = default;
 

vtr_flow/scripts/python_libs/vtr/util.py

@@ -146,6 +146,12 @@ def run_system_command(
         try:
             # Call the command
             stderr = None if self._valgrind else subprocess.STDOUT
+            '''
+            capnproto accesses PWD environment variable to learn about the current working directory.
+            However, subprocess.Popen() changes the working directory without updating this variable.
+            This can cause issues when a VTR task passes router lookahead or RR graph files to VPR.
+            PWD environment variable is updated manually to prevent capnproto from throwing exceptions.  
+            '''
             modified_environ = os.environ.copy()
             modified_environ['PWD'] = str(temp_dir)
             proc = subprocess.Popen(