Code review update: enhance more documentations. Added function level description for three routines in place.cpp: find_affected_sink_pins(), commit_td_cost(), invalidate_affected_connection_delays().

Author: Bill-hbrhbr

doc/src/vpr/command_line_usage.rst

@@ -715,7 +715,7 @@ If any of init_t, exit_t or alpha_t is specified, the user schedule, with a fixe
 .. option:: --place_quench_algorithm {bounding_box | criticality_timing | slack_timing}
 
     Controls the algorithm used by the placer during placement quench.
-    The algorithm options have identical functionality as the ones used by the option ``--place_algorithm``.
+    The algorithm options have identical functionality as the ones used by the option ``--place_algorithm``. If specified, it overrides the option ``--place_algorithm`` during placement quench.
 
     **Default:**  ``criticality_timing``
 

vpr/src/base/read_options.cpp

@@ -359,6 +359,13 @@ struct ParsePlaceAlgorithm {
         } else {
             std::stringstream msg;
             msg << "Invalid conversion from '" << str << "' to e_place_algorithm (expected one of: " << argparse::join(default_choices(), ", ") << ")";
+
+            //Deprecated option: "path_timing_driven" -> PATH_DRIVEN_TIMING_PLACE
+            //New option: "criticality_timing" -> CRITICALITY_TIMING_PLACE
+            if (str == "path_timing_driven") {
+                msg << "\nDeprecated option: 'path_timing_driven'. It has been renamed to 'criticality_timing'";
+            }
+
             conv_value.set_error(msg.str());
         }
         return conv_value;
@@ -1694,7 +1701,9 @@ argparse::ArgumentParser create_arg_parser(std::string prog_name, t_options& arg
 
     place_grp.add_argument<e_place_algorithm, ParsePlaceAlgorithm>(args.PlaceQuenchAlgorithm, "--place_quench_algorithm")
         .help(
-            "Controls which placement algorithm is used during placement quench. Valid options:\n"
+            "Controls which placement algorithm is used during placement quench.\n"
+            "If specified, it overrides the option --place_algorithm during placement quench.\n"
+            "Valid options:\n"
             " * bounding_box: Focuses purely on minimizing the bounding box wirelength of the circuit. Turns off timing analysis if specified.\n"
             " * criticality_timing: Focuses on minimizing both the wirelength and the connection timing costs (criticality * delay).\n"
             " * slack_timing: Focuses on improving the circuit slack values to reduce critical path delay.\n")

vpr/src/base/vpr_types.h

@@ -859,7 +859,7 @@ enum e_place_algorithm {
 /**
  * @brief Provides a wrapper around enum e_place_algorithm.
  *
- * Supports the method isTimingDriven(), which allows flexible updates
+ * Supports the method is_timing_driven(), which allows flexible updates
  * to the placer algorithms if more timing driven placement strategies
  * are added in tht future. This method is used across various placement
  * setup files, and it can be useful for major placer routines as well.
@@ -879,8 +879,14 @@ class t_place_algorithm {
     ~t_place_algorithm() = default;
 
     //Assignment operators
-    void operator=(const t_place_algorithm& rhs) { algo = rhs.algo; }
-    void operator=(e_place_algorithm rhs) { algo = rhs; }
+    t_place_algorithm& operator=(const t_place_algorithm& rhs) {
+        algo = rhs.algo;
+        return *this;
+    }
+    t_place_algorithm& operator=(e_place_algorithm rhs) {
+        algo = rhs;
+        return *this;
+    }
 
     //Equality operators
     bool operator==(const t_place_algorithm& rhs) const { return algo == rhs.algo; }
@@ -953,28 +959,31 @@ enum class e_place_delta_delay_algorithm {
  *   @param place_chan_width
  *              The channel width assumed if only one placement is performed.
  *   @param pad_loc_type
- *              Are pins FREE, fixed randomly, or fixed from a file.
- *   @param block_loc_type
- *              Are blocks fixed from a file.
+ *              Are pins FREE or fixed randomly.
  *   @param constraints_file
- *              File to read block locations from if block_loc_type is LOCKED.
+ *              File that specifies locations of locked down (constrained)
+ *              blocks for placement. Empty string means no constraints file.
  *   @param pad_loc_file
  *              File to read pad locations from if pad_loc_type is USER.
  *   @param place_freq
  *              Should the placement be skipped, done once, or done
  *              for each channel width in the binary search.
  *   @param recompute_crit_iter
  *              How many temperature stages pass before we recompute
- *              criticalities based on average point to point delay.
+ *              criticalities based on the current placement and its
+ *              estimated point-to-point delays.
  *   @param inner_loop_crit_divider
  *              (move_lim/inner_loop_crit_divider) determines how
  *              many inner_loop iterations pass before a recompute
  *              of criticalities is done.
  *   @param td_place_exp_first
  *              Exponent that is used in the CRITICALITY_TIMING_PLACE
- *              mode. It is the value that the crit_exponent starts at.
+ *              mode to specify the initial value of `crit_exponent`.
+ *              After we map the slacks to criticalities, this value
+ *              is used to `sharpen` the criticalities, making connections
+ *              with worse slacks more critical.
  *   @param td_place_exp_last
- *              Value that the criticality exponent will be at the end.
+ *              Value that the crit_exponent will be at the end.
  *   @param doPlacement
  *              True if placement is supposed to be done in the CAD flow.
  *              False if otherwise.

vpr/src/place/place.cpp

@@ -1653,9 +1653,13 @@ static e_move_result try_swap(float t,
         std::vector<ClusterPinId> sink_pins_affected;
         find_affected_sink_pins(blocks_affected, sink_pins_affected);
 
+        //For setup slack analysis, we first do a timing analysis to get the newest slack values
+        //resulted from the proposed block moves. If the move turns out to be accepted, we keep
+        //the updated slack values and commit the block moves. If rejected, we reject the proposed
+        //block moves and revert this timing analysis.
         if (place_algorithm == SLACK_TIMING_PLACE) {
-            //Invalidates timing of modified connections for incremental timing updates
-            //This routine relies on comparing proposed_connection_delay and connection_delay
+            //Gather all the connections with modified delays for incremental timing updates.
+            //This routine relies on comparing proposed_connection_delay and connection_delay.
             invalidate_affected_connection_delays(sink_pins_affected,
                                                   pin_timing_invalidator,
                                                   timing_info);
@@ -1749,7 +1753,9 @@ static e_move_result try_swap(float t,
                 comp_td_connection_delays(delay_model);
                 comp_td_costs(delay_model, *criticalities, &costs->timing_cost);
 
-                //Re-invalidate the affected sink pins
+                //Re-invalidate the affected sink pins since the proposed move is
+                //rejected, and the same blocks are reverted to their original
+                //positions. The affected sink pins should stay the same.
                 invalidate_affected_connection_delays(sink_pins_affected,
                                                       pin_timing_invalidator,
                                                       timing_info);
@@ -1946,19 +1952,23 @@ static void update_td_delta_costs(const PlaceDelayModel* delay_model,
     }
 }
 
+/**
+ * @brief Find all the sink pins with changed connection delays from the affected blocks.
+ *
+ * These sink pins will be passed into the pin_timing_invalidator for timing update.
+ * They will also be added to the pin invalidator when we wish to revert a timing update.
+ *
+ * It is possible that some connections may not have changed delay. For instance, if
+ * using a dx/dy delay model, this could occur if a sink moved to a new position with
+ * the same dx/dy from it's driver. To minimize work during the incremental STA update
+ * we do not invalidate such unchanged connections.
+ */
 static void find_affected_sink_pins(const t_pl_blocks_to_be_moved& blocks_affected,
                                     std::vector<ClusterPinId>& sink_pins_affected) {
     auto& cluster_ctx = g_vpr_ctx.clustering();
     auto& clb_nlist = cluster_ctx.clb_nlist;
 
     for (ClusterPinId clb_pin : blocks_affected.affected_pins) {
-        //It is possible that some connections may not have changed delay.(e.g.
-        //For instance, if using a dx/dy delay model, this could occur if a sink
-        //moved to a new position with the same dx/dy from it's driver.
-        //
-        //To minimize work during the incremental STA update we do not invalidate
-        //such unchanged connections.
-
         ClusterNetId net = clb_nlist.pin_net(clb_pin);
         int ipin = clb_nlist.pin_net_index(clb_pin);
 
@@ -2178,8 +2188,13 @@ static bool verify_connection_setup_slacks(const PlacerSetupSlacks* setup_slacks
     return true;
 }
 
-/* Update the connection_timing_cost values from the temporary *
- * values for all connections that have changed.               */
+/**
+ * @brief Update the connection_timing_cost values from the temporary
+ *        values for all connections that have/haven't changed.
+ *
+ * All the connections have already been gathered by blocks_affected.affected_pins
+ * after running the routine find_affected_nets_and_update_costs() in try_swap().
+ */
 static void commit_td_cost(const t_pl_blocks_to_be_moved& blocks_affected) {
     auto& cluster_ctx = g_vpr_ctx.clustering();
     auto& clb_nlist = cluster_ctx.clb_nlist;
@@ -2217,10 +2232,15 @@ static void revert_td_cost(const t_pl_blocks_to_be_moved& blocks_affected) {
 #endif
 }
 
-//Invalidates the delays of connections effected by the specified move
-//
-//Relies on proposed_connection_delay and connection_delay to detect
-//which connections have actually had their delay changed.
+/**
+ * @brief Invalidates the delays of connections effected by the specified move.
+ *
+ * Relies on find_affected_sink_pins() to find all the connections with different
+ * `proposed_connection_delay` and `connection_delay`.
+ *
+ * Invalidate all the timing graph edges associated with these sink pins via the
+ * ClusteredPinTimingInvalidator class.
+ */
 static void invalidate_affected_connection_delays(const std::vector<ClusterPinId>& sink_pins_affected,
                                                   ClusteredPinTimingInvalidator* pin_tedges_invalidator,
                                                   TimingInfo* timing_info) {
@@ -2229,12 +2249,6 @@ static void invalidate_affected_connection_delays(const std::vector<ClusterPinId
 
     //Invalidate timing graph edges affected by the move
     for (ClusterPinId clb_pin : sink_pins_affected) {
-        //It is possible that some connections may not have changed delay.
-        //For instance, if using a dx/dy delay model, this could occur if a sink
-        //moved to a new position with the same dx/dy from it's driver.
-        //
-        //To minimize work during the incremental STA update we do not invalidate
-        //such unchanged connections.
         pin_tedges_invalidator->invalidate_connection(clb_pin, timing_info);
     }
 }

vpr/src/place/timing_place.cpp

@@ -19,9 +19,6 @@
 
 #include "timing_info.h"
 
-///@brief Use an incremental approach to updating criticalities and setup slacks?
-static constexpr bool INCR_UPDATE_CRITICALITIES = true, INCR_UPDATE_SETUP_SLACKS = true;
-
 ///@brief Allocates space for the timing_place_crit_ data structure.
 PlacerCriticalities::PlacerCriticalities(const ClusteredNetlist& clb_nlist, const ClusteredPinAtomPinsLookup& netlist_pin_lookup)
     : clb_nlist_(clb_nlist)
@@ -48,7 +45,7 @@ void PlacerCriticalities::update_criticalities(const SetupTimingInfo* timing_inf
     }
 
     /* Determine what pins need updating */
-    if (!recompute_required && crit_exponent == last_crit_exponent_ && INCR_UPDATE_CRITICALITIES) {
+    if (!recompute_required && crit_exponent == last_crit_exponent_) {
         incr_update_criticalities(timing_info);
     } else {
         recompute_criticalities();
@@ -123,8 +120,11 @@ void PlacerCriticalities::recompute_criticalities() {
 }
 
 ///@brief Override the criticality of a particular connection.
-void PlacerCriticalities::set_criticality(ClusterNetId net_id, int ipin, float val) {
-    timing_place_crit_[net_id][ipin] = val;
+void PlacerCriticalities::set_criticality(ClusterNetId net_id, int ipin, float crit_val) {
+    VTR_ASSERT_SAFE_MSG(ipin > 0, "The pin should not be a driver pin (ipin = 0)");
+    VTR_ASSERT_SAFE_MSG(ipin < clb_nlist_.net_pins(net_id).size(), "The pin index in net should be smaller than fanout");
+
+    timing_place_crit_[net_id][ipin] = crit_val;
 }
 
 /**
@@ -163,7 +163,7 @@ void PlacerSetupSlacks::update_setup_slacks(const SetupTimingInfo* timing_info)
     }
 
     /* Determine what pins need updating */
-    if (!recompute_required && INCR_UPDATE_SETUP_SLACKS) {
+    if (!recompute_required) {
         incr_update_setup_slacks(timing_info);
     } else {
         recompute_setup_slacks();
@@ -223,8 +223,11 @@ void PlacerSetupSlacks::recompute_setup_slacks() {
 }
 
 ///@brief Override the setup slack of a particular connection.
-void PlacerSetupSlacks::set_setup_slack(ClusterNetId net_id, int ipin, float val) {
-    timing_place_setup_slacks_[net_id][ipin] = val;
+void PlacerSetupSlacks::set_setup_slack(ClusterNetId net_id, int ipin, float slack_val) {
+    VTR_ASSERT_SAFE_MSG(ipin > 0, "The pin should not be a driver pin (ipin = 0)");
+    VTR_ASSERT_SAFE_MSG(ipin < clb_nlist_.net_pins(net_id).size(), "The pin index in net should be smaller than fanout");
+
+    timing_place_setup_slacks_[net_id][ipin] = slack_val;
 }
 
 /**