[SQUASH ME] Removed Global Variables, Moved Legalizer Up, Started

AlexandreSinger · AlexandreSinger · commit df22e4cf8dc6 · 2024-08-28T15:45:46.000-04:00
Working on Free Methods.
diff --git a/vpr/src/base/clustered_netlist.cpp b/vpr/src/base/clustered_netlist.cpp
@@ -171,7 +171,8 @@ ClusterNetId ClusteredNetlist::create_net(const std::string& name) {
 
 void ClusteredNetlist::remove_block_impl(const ClusterBlockId blk_id) {
     //Remove & invalidate pointers
-    // FIXME: How will we free the pb?
+    // FIXME: This cannot set the pb to nullptr properly, causing it to double
+    //        free... Need some way to transfer ownership.
     /*
     free_pb(block_pbs_[blk_id]);
     delete block_pbs_[blk_id];
diff --git a/vpr/src/base/vpr_context.h b/vpr/src/base/vpr_context.h
@@ -313,27 +313,22 @@ struct ClusteringHelperContext : public Context {
     std::map<t_logical_block_type_ptr, size_t> num_used_type_instances;
 
     /// @brief Class used to construct legal clusters.
+    /// FIXME: Move this to the cluster context?
     ClusterLegalizer cluster_legalizer;
 
-    // Stats keeper for placement information during packing/clustering
-    t_cluster_placement_stats* cluster_placement_stats;
-
-    // total number of models in the architecture
-    int num_models;
-
+    // FIXME: This can be removed by adding an accessor to the cluster legalizer.
     int max_cluster_size;
-    t_pb_graph_node** primitives_list;
-
-    bool enable_pin_feasibility_filter;
-    int feasible_block_array_size;
 
     // total number of CLBs
     int total_clb_num;
 
     // A vector of routing resource nodes within each of logic cluster_ctx.blocks types [0 .. num_logical_block_type-1]
+    // FIXME: This is only used for a handoff between the vpr_setup and the packer.
+    //        This can be made cleaner.
     std::vector<t_lb_type_rr_node>* lb_type_rr_graphs;
 
     // the utilization of external input/output pins during packing (between 0 and 1)
+    // FIXME: This one is weird and may take some work to fix.
     t_ext_pin_util_targets target_external_pin_util;
 
     // During clustering, a block is related to un-clustered primitives with nets.
@@ -349,10 +344,6 @@ struct ClusteringHelperContext : public Context {
      * to different NoC groups can't be clustered with each other into the
      * same clustered block.*/
     vtr::vector<AtomBlockId, NocGroupId> atom_noc_grp_id;
-
-    ~ClusteringHelperContext() {
-        delete[] primitives_list;
-    }
 };
 
 /**
diff --git a/vpr/src/pack/cluster.cpp b/vpr/src/pack/cluster.cpp
@@ -75,7 +75,8 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
                                                          bool balance_block_type_utilization,
                                                          AttractionInfo& attraction_groups,
                                                          bool& floorplan_regions_overfull,
-                                                         t_clustering_data& clustering_data) {
+                                                         t_clustering_data& clustering_data,
+                                                         size_t num_models) {
     /* Does the actual work of clustering multiple netlist blocks *
      * into clusters.                                                  */
 
@@ -122,9 +123,6 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
     auto& cluster_ctx = g_vpr_ctx.mutable_clustering();
     auto& helper_ctx = g_vpr_ctx.mutable_cl_helper();
 
-    helper_ctx.enable_pin_feasibility_filter = packer_opts.enable_pin_feasibility_filter;
-    helper_ctx.feasible_block_array_size = packer_opts.feasible_block_array_size;
-
     std::shared_ptr<PreClusterDelayCalculator> clustering_delay_calc;
     std::shared_ptr<SetupTimingInfo> timing_info;
 
@@ -157,6 +155,8 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
 
     cluster_stats.num_molecules = prepacker.get_num_molecules();
 
+    // FIXME: The cluster legalizer has max_cluster size within it. We should
+    //        use it!
     get_max_cluster_size_and_pb_depth(helper_ctx.max_cluster_size, max_pb_depth);
 
     if (packer_opts.hill_climbing_flag) {
@@ -171,20 +171,8 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
 	check_for_duplicate_inputs ();
 #endif
 
-    // TODO: Try to separate from the helper context.
-    helper_ctx.cluster_legalizer.init(prepacker,
-                                      device_ctx.logical_block_types,
-                                      packer_opts.target_external_pin_util,
-                                      helper_ctx.lb_type_rr_graphs,
-                                      helper_ctx.num_models,
-                                      ClusterLegalizationStrategy::SKIP_INTRA_LB_ROUTE,
-                                      helper_ctx.enable_pin_feasibility_filter,
-                                      helper_ctx.feasible_block_array_size,
-                                      verbosity);
-    helper_ctx.cluster_placement_stats = helper_ctx.cluster_legalizer.cluster_placement_stats;
-
     alloc_and_init_clustering(max_molecule_stats,
-                              &(helper_ctx.primitives_list), prepacker,
+                              prepacker,
                               clustering_data, net_output_feeds_driving_block_input,
                               unclustered_list_head_size, cluster_stats.num_molecules);
 
@@ -237,6 +225,7 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
             if (is_cluster_legal)
                 break;
             // Set the legalization strategy of the cluster legalizer.
+            // FIXME: Pass in the cluster legalizer.
             helper_ctx.cluster_legalizer.set_legalization_strategy(strategy);
 
             // FIXME: Remove the reference to the clustered netlist. It is not
@@ -298,8 +287,7 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
                 /*it doesn't make sense to do a timing analysis here since there*
                  *is only one atom block clustered it would not change anything      */
             }
-            // FIXME: This is very gross and should be fixed.
-            // cur_cluster_placement_stats_ptr = &(helper_ctx.cluster_placement_stats[cluster_ctx.clb_nlist.block_type(clb_index)->index]);
+            // FIXME: This access should be a getter.
             cur_cluster_placement_stats_ptr = &(helper_ctx.cluster_legalizer.cluster_placement_stats[cluster_ctx.clb_nlist.block_type(clb_index)->index]);
             cluster_stats.num_unrelated_clustering_attempts = 0;
             next_molecule = get_molecule_for_cluster(cluster_ctx.clb_nlist.block_pb(clb_index),
diff --git a/vpr/src/pack/cluster.h b/vpr/src/pack/cluster.h
@@ -22,7 +22,8 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
                                                          bool balance_block_type_utilization,
                                                          AttractionInfo& attraction_groups,
                                                          bool& floorplan_regions_overfull,
-                                                         t_clustering_data& clustering_data);
+                                                         t_clustering_data& clustering_data,
+                                                         size_t num_models);
 
 int get_cluster_of_block(int blkidx);
 
diff --git a/vpr/src/pack/cluster_legalizer.cpp b/vpr/src/pack/cluster_legalizer.cpp
@@ -1161,6 +1161,7 @@ e_block_pack_status ClusterLegalizer::try_pack_molecule(t_pack_molecule* molecul
         if (atom_blk_id) {
             bool block_pack_noc_grp_status = check_cluster_noc_group(atom_blk_id,
                                                                      new_cluster_noc_grp_id,
+                                                                     // FIXME: Get this global access out of here!
                                                                      cl_helper_ctx.atom_noc_grp_id,
                                                                      log_verbosity);
             if (!block_pack_noc_grp_status) {
@@ -1412,7 +1413,6 @@ ClusterLegalizer::start_new_cluster(t_pack_molecule* molecule,
         // TODO: Move this to the destructor of the cluster class (however, this
         //       may cause issues so maybe just make it into a free method)...
         free_pb(new_cluster.pb);
-        // free_pb_stats_recursive(new_cluster.cluster_pb);
         delete new_cluster.pb;
         free_router_data(new_cluster.router_data);
         new_cluster_id = LegalizationClusterId::INVALID();
@@ -1436,8 +1436,9 @@ e_block_pack_status ClusterLegalizer::add_mol_to_cluster(t_pack_molecule* molecu
     LegalizationCluster& cluster = legalization_clusters[cluster_id];
     // FIXME: Handle destroyed clusters better.
     VTR_ASSERT(cluster.pb != nullptr && "Cannot destory an already destroyed cluster");
-    // FIXME: Why is this different from creating a new cluster?
-    t_ext_pin_util target_ext_pin_util = ext_pin_util_targets.get_pin_util(cluster.type->name);
+    // Set the target_external_pin_util.
+    // FIXME: Remove this access somehow.
+    t_ext_pin_util target_ext_pin_util = g_vpr_ctx.cl_helper().target_external_pin_util.get_pin_util(cluster.type->name);
     
     e_block_pack_status pack_status = try_pack_molecule(molecule,
                                                         cluster,
@@ -1483,11 +1484,7 @@ void ClusterLegalizer::destroy_cluster(LegalizationClusterId cluster_id) {
     //  Casting things to nullptr for safety just in case someone is trying to use it.
     // TODO: Make this a free method!!!
     //  This was stolen from the reset method.
-    // FIXME: There must be a memory leak somewhere. As we continue to clean
-    //        the code we will need to check valgrind.
-    //   - Need to take care of who owns the pb data.
-    // free_pb(cluster.cluster_pb);
-    // free_pb_stats_recursive(cluster.cluster_pb);
+    free_pb(cluster.pb);
     delete cluster.pb;
     cluster.pb = nullptr;
     free_router_data(cluster.router_data);
@@ -1503,7 +1500,6 @@ bool ClusterLegalizer::check_cluster_legality(LegalizationClusterId cluster_id)
 
 void ClusterLegalizer::init(const Prepacker& prepacker,
                             const std::vector<t_logical_block_type>& logical_block_types,
-                            const std::vector<std::string>& target_external_pin_util,
                             std::vector<t_lb_type_rr_node>* t_lb_type_rr_graphs,
                             size_t t_num_models,
                             ClusterLegalizationStrategy t_cluster_legalization_strategy,
@@ -1528,8 +1524,6 @@ void ClusterLegalizer::init(const Prepacker& prepacker,
     }
     // Calculate the max cluster size
     max_cluster_size = get_max_cluster_size(logical_block_types);
-    // Initialize the external pin util targets
-    ext_pin_util_targets = t_ext_pin_util_targets(target_external_pin_util);
     // Get a reference to the rr graphs.
     lb_type_rr_graphs = t_lb_type_rr_graphs;
     // Get the number of models in the architecture.
@@ -1567,15 +1561,12 @@ void ClusterLegalizer::reset() {
     // FIXME: It would make sense if this were to call the destroy method
     //        somehow.
     for (LegalizationCluster& cluster : legalization_clusters) {
-        // FIXME: Handle destroyed clusters better.
+        // FIXME: Should improve the interface for destroying a cluster.
         if (cluster.pb == nullptr)
             continue;
         // Free the cluster's data.
         // TODO: Make this a free method!!!
-        // FIXME: There must be a memory leak somewhere. As we continue to clean
-        //        the code we will need to check valgrind.
-        // free_pb(cluster.cluster_pb);
-        // free_pb_stats_recursive(cluster.cluster_pb);
+        free_pb(cluster.pb);
         delete cluster.pb;
         free_router_data(cluster.router_data);
     }
diff --git a/vpr/src/pack/cluster_legalizer.h b/vpr/src/pack/cluster_legalizer.h
@@ -102,7 +102,6 @@ class ClusterLegalizer {
      */
     void init(const Prepacker& prepacker,
               const std::vector<t_logical_block_type>& logical_block_types,
-              const std::vector<std::string>& target_external_pin_util,
               std::vector<t_lb_type_rr_node>* t_lb_type_rr_graphs,
               size_t t_num_models,
               ClusterLegalizationStrategy t_cluster_legalization_strategy,
@@ -214,6 +213,13 @@ class ClusterLegalizer {
         log_verbosity = verbosity;
     }
 
+    // FIXME: We should make a clean method to clean 1 or all clusters.
+    //  - free_pb_stats_recursive for example.
+    //  - free the router data.
+    //  - etc.
+    //  - This is to help reduce the memory overhead of the class without
+    //    breaking anything.
+
     void finalize();
 
     void reset();
@@ -228,6 +234,7 @@ class ClusterLegalizer {
     //        Need to cross-reference and see what matters and what doesnt for
     //        legalization. What variables do we touch and does this function
     //        touch it!
+    // Stats keeper for placement information during packing/clustering
     t_cluster_placement_stats* cluster_placement_stats = nullptr;
 
 private:
@@ -247,12 +254,11 @@ class ClusterLegalizer {
 
     size_t max_cluster_size;
 
-    t_ext_pin_util_targets ext_pin_util_targets;
-
     // TODO: This really should not be a pointer to a vector... I think this is
     //       meant to be a vector of vectors...
     std::vector<t_lb_type_rr_node>* lb_type_rr_graphs = nullptr;
 
+    // total number of models in the architecture.
     size_t num_models;
 
     /// @brief The current legalization strategy of the cluster legalizer.
diff --git a/vpr/src/pack/cluster_util.cpp b/vpr/src/pack/cluster_util.cpp
@@ -482,7 +482,6 @@ void add_molecule_to_pb_stats_candidates(t_pack_molecule* molecule,
 
 /*****************************************/
 void alloc_and_init_clustering(const t_molecule_stats& max_molecule_stats,
-                               t_pb_graph_node*** primitives_list,
                                const Prepacker& prepacker,
                                t_clustering_data& clustering_data,
                                std::unordered_map<AtomNetId, int>& net_output_feeds_driving_block_input,
@@ -539,21 +538,10 @@ void alloc_and_init_clustering(const t_molecule_stats& max_molecule_stats,
             }
         }
     }
-
-    /* alloc and load cluster placement info */
-    // *cluster_placement_stats = alloc_and_load_cluster_placement_stats();
-
-    /* alloc array that will store primitives that a molecule gets placed to,
-     * primitive_list is referenced by index, for example a atom block in index 2 of a molecule matches to a primitive in index 2 in primitive_list
-     * this array must be the size of the biggest molecule
-     */
-    size_t max_molecule_size = prepacker.get_max_molecule_size();
-    *primitives_list = new t_pb_graph_node*[max_molecule_size];
-    for (size_t i = 0; i < max_molecule_size; i++)
-        (*primitives_list)[i] = nullptr;
 }
 
 /*****************************************/
+// FIXME: This now has no users. Should move to legalizers clean method.
 void free_pb_stats_recursive(t_pb* pb) {
     int i, j;
     /* Releases all the memory used by clustering data structures.   */
@@ -937,7 +925,7 @@ void store_cluster_info_and_free(const t_packer_opts& packer_opts,
 
     //print clustering progress incrementally
     //print_pack_status(num_clb, num_molecules, num_molecules_processed, mols_since_last_print, device_ctx.grid.width(), device_ctx.grid.height());
-    free_pb_stats_recursive(cur_pb);
+    //free_pb_stats_recursive(cur_pb);
 }
 
 /* Free up data structures and requeue used molecules */
diff --git a/vpr/src/pack/cluster_util.h b/vpr/src/pack/cluster_util.h
@@ -134,7 +134,6 @@ void remove_molecule_from_pb_stats_candidates(t_pack_molecule* molecule,
                                               t_pb* pb);
 
 void alloc_and_init_clustering(const t_molecule_stats& max_molecule_stats,
-                               t_pb_graph_node*** primitives_list,
                                const Prepacker& prepacker,
                                t_clustering_data& clustering_data,
                                std::unordered_map<AtomNetId, int>& net_output_feeds_driving_block_input,
diff --git a/vpr/src/pack/pack.cpp b/vpr/src/pack/pack.cpp
@@ -1,5 +1,6 @@
 #include <unordered_set>
 
+#include "cluster_legalizer.h"
 #include "vpr_context.h"
 #include "vtr_assert.h"
 #include "vtr_log.h"
@@ -46,8 +47,7 @@ bool try_pack(t_packer_opts* packer_opts,
     VTR_LOG("Begin packing '%s'.\n", packer_opts->circuit_file_name.c_str());
 
     /* determine number of models in the architecture */
-    helper_ctx.num_models = count_models(user_models);
-    helper_ctx.num_models += count_models(library_models);
+    size_t num_models = count_models(user_models) + count_models(library_models);
 
     is_clock = alloc_and_load_is_clock();
     is_global.insert(is_clock.begin(), is_clock.end());
@@ -114,6 +114,16 @@ bool try_pack(t_packer_opts* packer_opts,
     while (true) {
         free_clustering_data(*packer_opts, clustering_data);
 
+        // Initialize the cluster legalizer.
+        helper_ctx.cluster_legalizer.init(atom_mutable_ctx.prepacker,
+                                          device_ctx.logical_block_types,
+                                          helper_ctx.lb_type_rr_graphs,
+                                          num_models,
+                                          ClusterLegalizationStrategy::SKIP_INTRA_LB_ROUTE,
+                                          packer_opts->enable_pin_feasibility_filter,
+                                          packer_opts->feasible_block_array_size,
+                                          packer_opts->pack_verbosity);
+
         //Cluster the netlist
         helper_ctx.num_used_type_instances = do_clustering(
             *packer_opts,
@@ -126,7 +136,8 @@ bool try_pack(t_packer_opts* packer_opts,
             balance_block_type_util,
             attraction_groups,
             floorplan_regions_overfull,
-            clustering_data);
+            clustering_data,
+            num_models);
 
         //Try to size/find a device
         bool fits_on_device = try_size_device_grid(*arch, helper_ctx.num_used_type_instances, packer_opts->target_device_utilization, packer_opts->device_layout);
@@ -227,8 +238,8 @@ bool try_pack(t_packer_opts* packer_opts,
         g_vpr_ctx.mutable_floorplanning().cluster_constraints.clear();
         //attraction_groups.reset_attraction_groups();
 
-        free_cluster_placement_stats(helper_ctx.cluster_placement_stats);
-        delete[] helper_ctx.primitives_list;
+        // Reset the cluster legalizer for re-clustering.
+        helper_ctx.cluster_legalizer.reset();
 
         ++pack_iteration;
     }
@@ -255,6 +266,9 @@ bool try_pack(t_packer_opts* packer_opts,
     VTR_LOG("IS THIS YOUR CARD?\n");
     free_clustering_data(*packer_opts, clustering_data);
 
+    // Clear the cluster legalizer.
+    helper_ctx.cluster_legalizer.reset();
+
     VTR_LOG("\n");
     VTR_LOG("Netlist conversion complete.\n");
     VTR_LOG("\n");
diff --git a/vpr/src/pack/re_cluster.cpp b/vpr/src/pack/re_cluster.cpp
@@ -60,8 +60,8 @@ bool move_mol_to_new_cluster(t_pack_molecule* molecule,
     is_created = start_new_cluster_for_mol(molecule,
                                            block_type,
                                            block_mode,
-                                           helper_ctx.feasible_block_array_size,
-                                           helper_ctx.enable_pin_feasibility_filter,
+                                           0, // helper_ctx.feasible_block_array_size,
+                                           false, // helper_ctx.enable_pin_feasibility_filter,
                                            new_clb,
                                            during_packing,
                                            verbosity,
diff --git a/vpr/src/pack/re_cluster_util.cpp b/vpr/src/pack/re_cluster_util.cpp
