[SQUASH ME] Moved Commit Lookahead Pins into legalizer and comments.

AlexandreSinger · AlexandreSinger · commit 4cea938226ec · 2024-08-28T13:06:46.000-04:00
diff --git a/vpr/src/pack/cluster_legalizer.cpp b/vpr/src/pack/cluster_legalizer.cpp
@@ -15,6 +15,7 @@
 #include <string>
 #include <tuple>
 #include <vector>
+#include "atom_lookup.h"
 #include "cluster_placement.h"
 #include "cluster_router.h"
 #include "globals.h"
@@ -971,6 +972,39 @@ static void revert_place_atom_block(const AtomBlockId blk_id,
     atom_ctx.lookup.set_atom_pb(blk_id, nullptr);
 }
 
+/* Speculation successful, commit input/output pins used */
+static void commit_lookahead_pins_used(t_pb* cur_pb) {
+    const t_pb_type* pb_type = cur_pb->pb_graph_node->pb_type;
+
+    if (pb_type->num_modes > 0 && cur_pb->name) {
+        for (int i = 0; i < cur_pb->pb_graph_node->num_input_pin_class; i++) {
+            VTR_ASSERT(cur_pb->pb_stats->lookahead_input_pins_used[i].size() <= (unsigned int)cur_pb->pb_graph_node->input_pin_class_size[i]);
+            for (size_t j = 0; j < cur_pb->pb_stats->lookahead_input_pins_used[i].size(); j++) {
+                VTR_ASSERT(cur_pb->pb_stats->lookahead_input_pins_used[i][j]);
+                cur_pb->pb_stats->input_pins_used[i].insert({j, cur_pb->pb_stats->lookahead_input_pins_used[i][j]});
+            }
+        }
+
+        for (int i = 0; i < cur_pb->pb_graph_node->num_output_pin_class; i++) {
+            VTR_ASSERT(cur_pb->pb_stats->lookahead_output_pins_used[i].size() <= (unsigned int)cur_pb->pb_graph_node->output_pin_class_size[i]);
+            for (size_t j = 0; j < cur_pb->pb_stats->lookahead_output_pins_used[i].size(); j++) {
+                VTR_ASSERT(cur_pb->pb_stats->lookahead_output_pins_used[i][j]);
+                cur_pb->pb_stats->output_pins_used[i].insert({j, cur_pb->pb_stats->lookahead_output_pins_used[i][j]});
+            }
+        }
+
+        if (cur_pb->child_pbs) {
+            for (int i = 0; i < pb_type->modes[cur_pb->mode].num_pb_type_children; i++) {
+                if (cur_pb->child_pbs[i]) {
+                    for (int j = 0; j < pb_type->modes[cur_pb->mode].pb_type_children[i].num_pb; j++) {
+                        commit_lookahead_pins_used(&cur_pb->child_pbs[i][j]);
+                    }
+                }
+            }
+        }
+    }
+}
+
 /**
  * Cleans up a pb after unsuccessful molecule packing
  *
@@ -1272,14 +1306,23 @@ e_block_pack_status ClusterLegalizer::try_pack_molecule(t_pack_molecule* molecul
                 cluster.molecules.insert(molecule);
 
                 for (int i = 0; i < molecule_size; i++) {
-                    if (molecule->atom_block_ids[i]) {
-                        /* invalidate all molecules that share atom block with current molecule */
-                        t_pack_molecule* cur_molecule = atom_ctx.prepacker.get_atom_molecule(molecule->atom_block_ids[i]);
-                        cur_molecule->valid = false;
-
-                        commit_primitive(cluster_placement_stats_ptr, primitives_list[i]);
-                    }
+                    AtomBlockId atom_blk_id = molecule->atom_block_ids[i];
+                    if (!atom_blk_id.is_valid())
+                        continue;
+
+                    /* invalidate all molecules that share atom block with current molecule */
+                    t_pack_molecule* cur_molecule = atom_ctx.prepacker.get_atom_molecule(atom_blk_id);
+                    // TODO: This should really be named better. Something like
+                    //       "is_clustered". and then it should be set to true.
+                    //       Right now, valid implies "not clustered" which is
+                    //       confusing.
+                    cur_molecule->valid = false;
+
+                    commit_primitive(cluster_placement_stats_ptr, primitives_list[i]);
                 }
+
+                // Update the lookahead pins used.
+                commit_lookahead_pins_used(cluster.pb);
             }
         }
 
@@ -1521,6 +1564,8 @@ void ClusterLegalizer::reset() {
     // Reset the molecule_cluster map
     molecule_cluster.clear();
     // Free and reset the legalization_clusters
+    // FIXME: It would make sense if this were to call the destroy method
+    //        somehow.
     for (LegalizationCluster& cluster : legalization_clusters) {
         // FIXME: Handle destroyed clusters better.
         if (cluster.pb == nullptr)
diff --git a/vpr/src/pack/cluster_legalizer.h b/vpr/src/pack/cluster_legalizer.h
@@ -46,16 +46,14 @@ enum class e_block_pack_status {
  * @brief A struct containing information about the cluster.
  *
  * This contains necessary information for legalizing a cluster.
- *
- * FIXME: The "cluster_" prefix for the members is a bit unnecessary. remove.
  */
 struct LegalizationCluster {
     /// @brief A list of the molecules in the cluster. By design, a cluster will
     ///        only contain molecules which have been previously legalized into
     ///        the cluster.
     std::set<t_pack_molecule*> molecules;
 
-    /// @brief The physical block this cluster represents.
+    /// @brief The logical block of this cluster.
     /// FIXME: We should be more careful with how this is allocated. Instead of
     ///        pointers, we really should use IDs and store them in a standard
     ///        container.
@@ -66,17 +64,18 @@ struct LegalizationCluster {
 
     /// @brief The partition region of legal positions this cluster can be placed.
     ///        Used to detect if a molecule can physically be placed in a cluster.
+    ///        It is derived from the partition region constraints on the atoms
+    ///        in the cluster (not fundamental but good for performance).
     PartitionRegion pr;
 
     /// @brief The NoC group that this cluster is a part of. Is used to check if
     ///        a candidate primitive is in the same NoC group as the atom blocks
-    ///        that have already been added to the primitive.
+    ///        that have already been added to the primitive. This can be helpful
+    ///        for optimization.
     NocGroupId noc_grp_id;
 
     /// @brief The router data of the intra lb router used for this cluster.
-    // NOTE: This may not actually need to be kept per cluster... Not sure...
-    // I think this is passed into the subroutines to prevent repetitive work
-    // in the lb router; but I am not sure...
+    /// TODO: Improve this description.
     t_lb_router_data* router_data;
 };
 
diff --git a/vpr/src/pack/cluster_util.cpp b/vpr/src/pack/cluster_util.cpp
@@ -1284,8 +1284,6 @@ void update_cluster_stats(const t_pack_molecule* molecule,
 
         update_total_gain(alpha, beta, timing_driven, connection_driven,
                           atom_pb->parent_pb, attraction_groups);
-
-        commit_lookahead_pins_used(cb);
     }
 
     // if this molecule came from the transitive fanout candidates remove it
@@ -2070,39 +2068,6 @@ float get_molecule_gain(t_pack_molecule* molecule, std::map<AtomBlockId, float>&
     return gain;
 }
 
-/* Speculation successful, commit input/output pins used */
-void commit_lookahead_pins_used(t_pb* cur_pb) {
-    const t_pb_type* pb_type = cur_pb->pb_graph_node->pb_type;
-
-    if (pb_type->num_modes > 0 && cur_pb->name) {
-        for (int i = 0; i < cur_pb->pb_graph_node->num_input_pin_class; i++) {
-            VTR_ASSERT(cur_pb->pb_stats->lookahead_input_pins_used[i].size() <= (unsigned int)cur_pb->pb_graph_node->input_pin_class_size[i]);
-            for (size_t j = 0; j < cur_pb->pb_stats->lookahead_input_pins_used[i].size(); j++) {
-                VTR_ASSERT(cur_pb->pb_stats->lookahead_input_pins_used[i][j]);
-                cur_pb->pb_stats->input_pins_used[i].insert({j, cur_pb->pb_stats->lookahead_input_pins_used[i][j]});
-            }
-        }
-
-        for (int i = 0; i < cur_pb->pb_graph_node->num_output_pin_class; i++) {
-            VTR_ASSERT(cur_pb->pb_stats->lookahead_output_pins_used[i].size() <= (unsigned int)cur_pb->pb_graph_node->output_pin_class_size[i]);
-            for (size_t j = 0; j < cur_pb->pb_stats->lookahead_output_pins_used[i].size(); j++) {
-                VTR_ASSERT(cur_pb->pb_stats->lookahead_output_pins_used[i][j]);
-                cur_pb->pb_stats->output_pins_used[i].insert({j, cur_pb->pb_stats->lookahead_output_pins_used[i][j]});
-            }
-        }
-
-        if (cur_pb->child_pbs) {
-            for (int i = 0; i < pb_type->modes[cur_pb->mode].num_pb_type_children; i++) {
-                if (cur_pb->child_pbs[i]) {
-                    for (int j = 0; j < pb_type->modes[cur_pb->mode].pb_type_children[i].num_pb; j++) {
-                        commit_lookahead_pins_used(&cur_pb->child_pbs[i][j]);
-                    }
-                }
-            }
-        }
-    }
-}
-
 /**
  * Score unclustered atoms that are two hops away from current cluster
  * For example, consider a cluster that has a FF feeding an adder in another
diff --git a/vpr/src/pack/cluster_util.h b/vpr/src/pack/cluster_util.h
@@ -143,8 +143,6 @@ void alloc_and_init_clustering(const t_molecule_stats& max_molecule_stats,
 
 void free_pb_stats_recursive(t_pb* pb);
 
-void commit_lookahead_pins_used(t_pb* cur_pb);
-
 t_pack_molecule* get_molecule_by_num_ext_inputs(const int ext_inps,
                                                 const enum e_removal_policy remove_flag,
                                                 t_cluster_placement_stats* cluster_placement_stats_ptr,
