[Packer] Updated Candidate Selector Class Based on VB Comments

AlexandreSinger · AlexandreSinger · commit 3defccfb11f0 · 2025-01-13T17:20:53.000-05:00
diff --git a/vpr/src/pack/greedy_candidate_selector.cpp b/vpr/src/pack/greedy_candidate_selector.cpp
@@ -25,6 +25,8 @@
  * total_block_gain
  * + molecule_base_gain*some_factor
  * - introduced_input_nets_of_unrelated_blocks_pulled_in_by_molecule*some_other_factor
+ *
+ * TODO: Confirm that this comment is correct.
  */
 static float get_molecule_gain(t_pack_molecule* molecule,
                                ClusterGainStats& cluster_gain_stats,
@@ -277,11 +279,11 @@ void GreedyCandidateSelector::mark_and_update_partial_gain(
             for (AtomPinId pin_id : pins) {
                 AtomBlockId blk_id = atom_netlist_.pin_block(pin_id);
                 if (!cluster_legalizer.is_atom_clustered(blk_id)) {
-                    if (cluster_gain_stats.sharinggain.count(blk_id) == 0) {
+                    if (cluster_gain_stats.sharing_gain.count(blk_id) == 0) {
                         cluster_gain_stats.marked_blocks.push_back(blk_id);
-                        cluster_gain_stats.sharinggain[blk_id] = 1;
+                        cluster_gain_stats.sharing_gain[blk_id] = 1;
                     } else {
-                        cluster_gain_stats.sharinggain[blk_id]++;
+                        cluster_gain_stats.sharing_gain[blk_id]++;
                     }
                 }
             }
@@ -333,7 +335,7 @@ void GreedyCandidateSelector::update_connection_gain_values(
                                        const ClusterLegalizer& cluster_legalizer,
                                        e_net_relation_to_clustered_block net_relation_to_clustered_block) {
 
-    /*This function is called when the connectiongain values on the net net_id*
+    /*This function is called when the connection_gain values on the net net_id
      *require updating.   */
 
     // Atom Context used to lookup the atom pb.
@@ -368,33 +370,33 @@ void GreedyCandidateSelector::update_connection_gain_values(
                 /* TODO: Gain function accurate only if net has one connection to block,
                  * TODO: Should we handle case where net has multi-connection to block?
                  *       Gain computation is only off by a bit in this case */
-                if (cluster_gain_stats.connectiongain.count(blk_id) == 0) {
-                    cluster_gain_stats.connectiongain[blk_id] = 0;
+                if (cluster_gain_stats.connection_gain.count(blk_id) == 0) {
+                    cluster_gain_stats.connection_gain[blk_id] = 0;
                 }
 
                 if (num_internal_connections > 1) {
-                    cluster_gain_stats.connectiongain[blk_id] -= 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 1 + 0.1);
+                    cluster_gain_stats.connection_gain[blk_id] -= 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 1 + 0.1);
                 }
-                cluster_gain_stats.connectiongain[blk_id] += 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 0.1);
+                cluster_gain_stats.connection_gain[blk_id] += 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 0.1);
             }
         }
     }
 
     if (net_relation_to_clustered_block == e_net_relation_to_clustered_block::INPUT) {
-        /*Calculate the connectiongain for the atom block which is driving *
+        /*Calculate the connection_gain for the atom block which is driving *
          *the atom net that is an input to an atom block in the cluster */
 
         AtomPinId driver_pin_id = atom_netlist_.net_driver(net_id);
         AtomBlockId blk_id = atom_netlist_.pin_block(driver_pin_id);
 
         if (!cluster_legalizer.is_atom_clustered(blk_id)) {
-            if (cluster_gain_stats.connectiongain.count(blk_id) == 0) {
-                cluster_gain_stats.connectiongain[blk_id] = 0;
+            if (cluster_gain_stats.connection_gain.count(blk_id) == 0) {
+                cluster_gain_stats.connection_gain[blk_id] = 0;
             }
             if (num_internal_connections > 1) {
-                cluster_gain_stats.connectiongain[blk_id] -= 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 0.1 + 1);
+                cluster_gain_stats.connection_gain[blk_id] -= 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 0.1 + 1);
             }
-            cluster_gain_stats.connectiongain[blk_id] += 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 0.1);
+            cluster_gain_stats.connection_gain[blk_id] += 1 / (float)(num_open_connections + 1.5 * num_stuck_connections + 0.1);
         }
     }
 }
@@ -405,7 +407,7 @@ void GreedyCandidateSelector::update_timing_gain_values(
                                    const ClusterLegalizer& cluster_legalizer,
                                    e_net_relation_to_clustered_block net_relation_to_clustered_block) {
 
-    /*This function is called when the timing_gain values on the atom net*
+    /*This function is called when the timing_gain values on the atom net
      *net_id requires updating.   */
 
     /* Check if this atom net lists its driving atom block twice.  If so, avoid  *
@@ -419,13 +421,13 @@ void GreedyCandidateSelector::update_timing_gain_values(
         for (AtomPinId pin_id : pins) {
             AtomBlockId blk_id = atom_netlist_.pin_block(pin_id);
             if (!cluster_legalizer.is_atom_clustered(blk_id)) {
-                double timinggain = timing_info_.setup_pin_criticality(pin_id);
+                double timing_gain = timing_info_.setup_pin_criticality(pin_id);
 
-                if (cluster_gain_stats.timinggain.count(blk_id) == 0) {
-                    cluster_gain_stats.timinggain[blk_id] = 0;
+                if (cluster_gain_stats.timing_gain.count(blk_id) == 0) {
+                    cluster_gain_stats.timing_gain[blk_id] = 0;
                 }
-                if (timinggain > cluster_gain_stats.timinggain[blk_id])
-                    cluster_gain_stats.timinggain[blk_id] = timinggain;
+                if (timing_gain > cluster_gain_stats.timing_gain[blk_id])
+                    cluster_gain_stats.timing_gain[blk_id] = timing_gain;
             }
         }
     }
@@ -439,13 +441,13 @@ void GreedyCandidateSelector::update_timing_gain_values(
 
         if (!cluster_legalizer.is_atom_clustered(new_blk_id)) {
             for (AtomPinId pin_id : atom_netlist_.net_sinks(net_id)) {
-                double timinggain = timing_info_.setup_pin_criticality(pin_id);
+                double timing_gain = timing_info_.setup_pin_criticality(pin_id);
 
-                if (cluster_gain_stats.timinggain.count(new_blk_id) == 0) {
-                    cluster_gain_stats.timinggain[new_blk_id] = 0;
+                if (cluster_gain_stats.timing_gain.count(new_blk_id) == 0) {
+                    cluster_gain_stats.timing_gain[new_blk_id] = 0;
                 }
-                if (timinggain > cluster_gain_stats.timinggain[new_blk_id])
-                    cluster_gain_stats.timinggain[new_blk_id] = timinggain;
+                if (timing_gain > cluster_gain_stats.timing_gain[new_blk_id])
+                    cluster_gain_stats.timing_gain[new_blk_id] = timing_gain;
             }
         }
     }
@@ -456,13 +458,13 @@ void GreedyCandidateSelector::update_total_gain(ClusterGainStats& cluster_gain_s
     AttractGroupId cluster_att_grp_id = cluster_gain_stats.attraction_grp_id;
 
     for (AtomBlockId blk_id : cluster_gain_stats.marked_blocks) {
-        //Initialize connectiongain and sharinggain if
+        //Initialize connection_gain and sharing_gain if
         //they have not previously been updated for the block
-        if (cluster_gain_stats.connectiongain.count(blk_id) == 0) {
-            cluster_gain_stats.connectiongain[blk_id] = 0;
+        if (cluster_gain_stats.connection_gain.count(blk_id) == 0) {
+            cluster_gain_stats.connection_gain[blk_id] = 0;
         }
-        if (cluster_gain_stats.sharinggain.count(blk_id) == 0) {
-            cluster_gain_stats.sharinggain[blk_id] = 0;
+        if (cluster_gain_stats.sharing_gain.count(blk_id) == 0) {
+            cluster_gain_stats.sharing_gain[blk_id] = 0;
         }
 
         AttractGroupId atom_grp_id = attraction_groups.get_atom_attraction_group(blk_id);
@@ -484,18 +486,18 @@ void GreedyCandidateSelector::update_total_gain(ClusterGainStats& cluster_gain_s
         if (packer_opts_.connection_driven) {
             /*try to absorb as many connections as possible*/
             cluster_gain_stats.gain[blk_id] = ((1 - packer_opts_.beta)
-                                                  * (float)cluster_gain_stats.sharinggain[blk_id]
-                                              + packer_opts_.beta * (float)cluster_gain_stats.connectiongain[blk_id])
+                                                  * (float)cluster_gain_stats.sharing_gain[blk_id]
+                                              + packer_opts_.beta * (float)cluster_gain_stats.connection_gain[blk_id])
                                              / (num_used_pins);
         } else {
-            cluster_gain_stats.gain[blk_id] = ((float)cluster_gain_stats.sharinggain[blk_id])
+            cluster_gain_stats.gain[blk_id] = ((float)cluster_gain_stats.sharing_gain[blk_id])
                                              / (num_used_pins);
         }
 
         /* Add in timing driven cost into cost function */
         if (packer_opts_.timing_driven) {
             cluster_gain_stats.gain[blk_id] = packer_opts_.alpha
-                                                 * cluster_gain_stats.timinggain[blk_id]
+                                                 * cluster_gain_stats.timing_gain[blk_id]
                                              + (1.0 - packer_opts_.alpha) * (float)cluster_gain_stats.gain[blk_id];
         }
     }
@@ -761,7 +763,7 @@ void GreedyCandidateSelector::add_cluster_molecule_candidates_by_attraction_grou
         return;
     }
 
-    if (num_available_atoms < 500) {
+    if (num_available_atoms < attraction_group_num_atoms_threshold_) {
         for (AtomBlockId atom_id : available_atoms) {
             //Only consider molecules that are unpacked and of the correct type
             t_pack_molecule* molecule = prepacker.get_atom_molecule(atom_id);
@@ -780,7 +782,10 @@ void GreedyCandidateSelector::add_cluster_molecule_candidates_by_attraction_grou
     int min = 0;
     int max = num_available_atoms - 1;
 
-    for (int j = 0; j < 500; j++) {
+    for (int j = 0; j < attraction_group_num_atoms_threshold_; j++) {
+        // FIXME: This is a non-deterministic random number generator and it is
+        //        overkill to what this needs to be. Should use vtr::irand which
+        //        would be faster.
         std::random_device rd;
         std::mt19937 gen(rd());
         std::uniform_int_distribution<> distr(min, max);
diff --git a/vpr/src/pack/greedy_candidate_selector.h b/vpr/src/pack/greedy_candidate_selector.h
@@ -39,16 +39,15 @@ struct ClusterGainStats {
     /// @brief Attraction (inverse of cost) function.
     std::unordered_map<AtomBlockId, float> gain;
 
-    /// @brief The timing criticality score of this atom cluster_ctx.blocks.
+    /// @brief The timing criticality score of this atom.
     ///        Determined by the most critical atom net between this atom
-    ///        cluster_ctx.blocks and any atom cluster_ctx.blocks in the current
-    ///        pb.
-    std::unordered_map<AtomBlockId, float> timinggain;
+    ///        and any atom in the current pb.
+    std::unordered_map<AtomBlockId, float> timing_gain;
     /// @brief Weighted sum of connections to attraction function.
-    std::unordered_map<AtomBlockId, float> connectiongain;
-    /// @brief How many nets on an atom cluster_ctx.blocks are already in the
-    ///        pb under consideration.
-    std::unordered_map<AtomBlockId, float> sharinggain;
+    std::unordered_map<AtomBlockId, float> connection_gain;
+    /// @brief How many nets on an atom are already in the pb under
+    ///        consideration.
+    std::unordered_map<AtomBlockId, float> sharing_gain;
 
     /// @brief Stores the number of times atoms have failed to be packed into
     ///        the cluster.
@@ -57,7 +56,9 @@ struct ClusterGainStats {
     /// has failed to be packed into the cluster.
     std::unordered_map<AtomBlockId, int> atom_failures;
 
-    /// @brief List of nets with the num_pins_of_net_in_pb and gain entries altered.
+    /// @brief List of nets with the num_pins_of_net_in_pb and gain entries
+    ///        altered (i.e. have some gain-related connection to the current
+    ///        cluster).
     std::vector<AtomNetId> marked_nets;
     /// @brief List of blocks with the num_pins_of_net_in_pb and gain entries altered.
     std::vector<AtomBlockId> marked_blocks;
@@ -69,7 +70,7 @@ struct ClusterGainStats {
     ///        determine next candidate molecule then explore molecules on
     ///        transitive fanout.
     bool explore_transitive_fanout;
-    /// @brief Holding trasitive fanout candidates key: root block id of the
+    /// @brief Holding transitive fanout candidates key: root block id of the
     ///        molecule, value: pointer to the molecule.
     // TODO: This should be an unordered map, unless stability is desired.
     std::map<AtomBlockId, t_pack_molecule*> transitive_fanout_candidates;
@@ -87,7 +88,7 @@ struct ClusterGainStats {
     /// Sorted in ascending gain order so that the last cluster_ctx.blocks is
     /// the most desirable (this makes it easy to pop blocks off the list.
     std::vector<t_pack_molecule*> feasible_blocks;
-    int num_feasible_blocks; /* [0..num_marked_models-1] */
+    int num_feasible_blocks;
 };
 
 /**
@@ -139,10 +140,16 @@ class GreedyCandidateSelector {
     static constexpr int AAPACK_MAX_HIGH_FANOUT_EXPLORE = 10;
 
     /// @brief When investigating transitive fanout connections in packing,
-    ///        consider a maximum of this many molecule s, must be less than
+    ///        consider a maximum of this many molecules, must be less than
     ///        packer_opts.feasible_block_array_size.
     static constexpr int AAPACK_MAX_TRANSITIVE_EXPLORE = 40;
 
+    /// @brief When adding cluster molecule candidates by attraction groups,
+    ///        only investigate this many candidates. Some attraction groups can
+    ///        get very large; so this threshold decides when to explore all
+    ///        atoms in the group, or a randomly selected number of them.
+    static constexpr int attraction_group_num_atoms_threshold_ = 500;
+
 public:
     ~GreedyCandidateSelector();
 
@@ -181,9 +188,9 @@ class GreedyCandidateSelector {
      *  @param net_output_feeds_driving_block_input
      *              The set of nets whose output feeds the block that drives
      *              itself. This may cause double-counting in the gain
-     *              calculations and need to be handled special.
+     *              calculations and needs special handling.
      *  @param timing_info
-     *              Setup timing info for this Atom Netlist. Used to incorperate
+     *              Setup timing info for this Atom Netlist. Used to incorporate
      *              timing / criticality into the gain calculation.
      *  @param log_verbosity
      *              The verbosity of log messages in the candidate selector.
@@ -265,8 +272,6 @@ class GreedyCandidateSelector {
      *  @param failed_mol
      *              The molecule that failed to pack into the cluster.
      */
-    // Update the cluster gain stats after a candidate was unsuccessfully
-    // clustered into the current cluster.
     void update_cluster_gain_stats_candidate_failed(
                                         ClusterGainStats& cluster_gain_stats,
                                         t_pack_molecule* failed_mol);
@@ -304,7 +309,9 @@ class GreedyCandidateSelector {
      * This should be called after all molecules have been packed into a cluster.
      *
      * This updates internal lookup tables in the candidate selector. For
-     * example, inter-clb nets.
+     * example, what inter-clb nets exist on a cluster are stored by this
+     * routine to make later transistive gain function calculations more
+     * efficient.
      *
      *  @param cluster_gain_stats
      *              The cluster gain stats for the cluster to finalize.
@@ -351,7 +358,7 @@ class GreedyCandidateSelector {
                                       e_net_relation_to_clustered_block net_relation_to_clustered_block);
 
     /**
-     * @brief Updates the connectiongain in the cluster_gain_stats.
+     * @brief Updates the connection_gain in the cluster_gain_stats.
      */
     void update_connection_gain_values(ClusterGainStats& cluster_gain_stats,
                                        AtomNetId net_id,
@@ -360,7 +367,7 @@ class GreedyCandidateSelector {
                                        e_net_relation_to_clustered_block net_relation_to_clustered_block);
 
     /**
-     * Updates the timinggain in the cluster_gain_stats.
+     * Updates the timing_gain in the cluster_gain_stats.
      */
     void update_timing_gain_values(ClusterGainStats& cluster_gain_stats,
                                    AtomNetId net_id,
@@ -369,8 +376,8 @@ class GreedyCandidateSelector {
 
     /**
      * @brief Updates the total gain array to reflect the desired tradeoff
-     *        between input sharing (sharinggain) and path_length minimization
-     *        (timinggain) input each time a new molecule is added to the
+     *        between input sharing (sharing_gain) and path_length minimization
+     *        (timing_gain) input each time a new molecule is added to the
      *        cluster.
      */
     void update_total_gain(ClusterGainStats& cluster_gain_stats,
@@ -439,8 +446,8 @@ class GreedyCandidateSelector {
      *
      * Attraction groups can be very large, so we only add some randomly
      * selected molecules for efficiency if the number of atoms in the group is
-     * greater than 500. Therefore, the molecules added to the candidates will
-     * vary each time you call this function.
+     * greater than some threshold. Therefore, the molecules added to the
+     * candidates will vary each time you call this function.
      */
     void add_cluster_molecule_candidates_by_attraction_group(
                                 ClusterGainStats& cluster_gain_stats,
@@ -450,7 +457,7 @@ class GreedyCandidateSelector {
                                 AttractionInfo& attraction_groups);
 
     /**
-     * @brief Finds a molecule to propose which is unrelated by may be good to
+     * @brief Finds a molecule to propose which is unrelated but may be good to
      *        cluster.
      */
     t_pack_molecule* get_unrelated_candidate_for_cluster(
@@ -473,7 +480,8 @@ class GreedyCandidateSelector {
     /// @brief The verbosity of log messages in the candidate selector.
     const int log_verbosity_;
 
-    /// @brief Pre-computed logical block types for each model in the architecture.
+    /// @brief Pre-computed vector of logical block types that could implement
+    ///        the given model in the architecture.
     const std::map<const t_model*, std::vector<t_logical_block_type_ptr>>& primitive_candidate_block_types_;
 
     /// @brief The high-fanout thresholds per logical block type. Used to ignore
@@ -500,7 +508,9 @@ class GreedyCandidateSelector {
     ///        transitive candidates.
     vtr::vector<LegalizationClusterId, std::vector<AtomNetId>> clb_inter_blk_nets_;
 
-    /// @brief Data pre-computed to help select unrelated molecules.
+    /// @brief Data pre-computed to help select unrelated molecules. This is a
+    ///        list of list of molecules sorted by their gain, where the first
+    ///        dimension is the number of external outputs of the molecule.
     std::vector<std::vector<t_pack_molecule *>> unrelated_clustering_data_;
 
     /// @brief A count on the number of unrelated clustering attempts which
diff --git a/vpr/src/pack/prepack.cpp b/vpr/src/pack/prepack.cpp
@@ -1729,7 +1729,7 @@ void Prepacker::init(const AtomNetlist& atom_nlist, const std::vector<t_logical_
     }
 }
 
-// TODO: Since this is constant per moleucle, it may make sense to precompute
+// TODO: Since this is constant per molecule, it may make sense to precompute
 //       this information and store it in the prepacker class. This may be
 //       expensive to calculate for large molecules.
 t_molecule_stats Prepacker::calc_molecule_stats(const t_pack_molecule* molecule,
diff --git a/vpr/src/pack/prepack.h b/vpr/src/pack/prepack.h

Original file line number	Diff line number	Diff line change
`@@ -1729,7 +1729,7 @@ void Prepacker::init(const AtomNetlist& atom_nlist, const std::vector<t_logical_`
`1729`	`1729`	`}`
`1730`	`1730`	`}`
`1731`	`1731`
`1732`		`-// TODO: Since this is constant per moleucle, it may make sense to precompute`
	`1732`	`+// TODO: Since this is constant per molecule, it may make sense to precompute`
`1733`	`1733`	`// this information and store it in the prepacker class. This may be`
`1734`	`1734`	`// expensive to calculate for large molecules.`
`1735`	`1735`	`t_molecule_stats Prepacker::calc_molecule_stats(const t_pack_molecule* molecule,`