[SQUASH ME] More Comments and Improvements

AlexandreSinger · AlexandreSinger · commit 38124064ebd6 · 2024-08-27T21:09:58.000-04:00
diff --git a/vpr/src/base/vpr_types.h b/vpr/src/base/vpr_types.h
@@ -168,15 +168,6 @@ enum class e_cluster_seed {
     BLEND2
 };
 
-enum class e_block_pack_status {
-    BLK_PASSED,
-    BLK_FAILED_FEASIBLE,
-    BLK_FAILED_ROUTE,
-    BLK_FAILED_FLOORPLANNING,
-    BLK_FAILED_NOC_GROUP,
-    BLK_STATUS_UNDEFINED
-};
-
 struct t_ext_pin_util {
     t_ext_pin_util() = default;
     t_ext_pin_util(float in, float out)
diff --git a/vpr/src/pack/cluster.cpp b/vpr/src/pack/cluster.cpp
@@ -100,7 +100,7 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
     t_cluster_progress_stats cluster_stats;
 
     //int num_molecules, num_molecules_processed, mols_since_last_print, blocks_since_last_analysis,
-    int num_blocks_hill_added, max_pb_depth, detailed_routing_stage;
+    int num_blocks_hill_added, max_pb_depth;
 
     const int verbosity = packer_opts.pack_verbosity;
 
@@ -177,7 +177,7 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
                                       packer_opts.target_external_pin_util,
                                       helper_ctx.lb_type_rr_graphs,
                                       helper_ctx.num_models,
-                                      e_detailed_routing_stages::E_DETAILED_ROUTE_AT_END_ONLY,
+                                      ClusterLegalizationStrategy::SKIP_INTRA_LB_ROUTE,
                                       helper_ctx.enable_pin_feasibility_filter,
                                       helper_ctx.feasible_block_array_size,
                                       verbosity);
@@ -224,8 +224,20 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
     while (istart != nullptr) {
         bool is_cluster_legal = false;
         int saved_seed_index = seed_index;
-        for (detailed_routing_stage = (int)E_DETAILED_ROUTE_AT_END_ONLY; !is_cluster_legal && detailed_routing_stage != (int)E_DETAILED_ROUTE_INVALID; detailed_routing_stage++) {
-            helper_ctx.cluster_legalizer.set_detailed_routing_stage(detailed_routing_stage);
+        // The basic algorithm:
+        // 1) Try to put all the molecules in that you can without doing the
+        //    full intra-lb route. Then do full legalization at the end.
+        // 2) If the legalization at the end fails, try again, but this time
+        //    do full legalization for each molecule added to the cluster.
+        const ClusterLegalizationStrategy legalization_strategies[] = {ClusterLegalizationStrategy::SKIP_INTRA_LB_ROUTE,
+                                                                       ClusterLegalizationStrategy::FULL};
+        for (const ClusterLegalizationStrategy strategy : legalization_strategies) {
+            // If the cluster is legal, no need to try a stronger cluster legalizer
+            // mode.
+            if (is_cluster_legal)
+                break;
+            // Set the legalization strategy of the cluster legalizer.
+            helper_ctx.cluster_legalizer.set_legalization_strategy(strategy);
 
             // FIXME: Remove the reference to the clustered netlist. It is not
             //        even correct.
@@ -349,10 +361,15 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
                                  primitive_candidate_block_types);
             }
 
-            if (detailed_routing_stage == (int)E_DETAILED_ROUTE_AT_END_ONLY) {
-                is_cluster_legal = helper_ctx.cluster_legalizer.check_cluster_legality(legalization_cluster_id);
-            } else {
+            if (strategy == ClusterLegalizationStrategy::FULL) {
+                // If the legalizer fully legalized for every molecule added,
+                // the cluster should be legal.
                 is_cluster_legal = true;
+            } else {
+                // If the legalizer did not check everything for every molecule,
+                // need to check that the full cluster is legal (need to perform
+                // intra-lb routing).
+                is_cluster_legal = helper_ctx.cluster_legalizer.check_cluster_legality(legalization_cluster_id);
             }
 
             if (is_cluster_legal) {
diff --git a/vpr/src/pack/cluster_legalizer.cpp b/vpr/src/pack/cluster_legalizer.cpp
diff --git a/vpr/src/pack/cluster_legalizer.h b/vpr/src/pack/cluster_legalizer.h
@@ -15,7 +15,6 @@ class t_ext_pin_util_targets;
 class t_pack_molecule;
 class t_pb;
 class t_pb_graph_node;
-enum class e_block_pack_status;
 struct t_ext_pin_util;
 struct t_lb_router_data;
 
@@ -25,11 +24,22 @@ struct t_lb_router_data;
 struct legalization_cluster_id_tag;
 typedef vtr::StrongId<legalization_cluster_id_tag, size_t> LegalizationClusterId;
 
-// FIXME: Make this an enum class and use proper format.
-enum e_detailed_routing_stages {
-    E_DETAILED_ROUTE_AT_END_ONLY = 0,
-    E_DETAILED_ROUTE_FOR_EACH_ATOM,
-    E_DETAILED_ROUTE_INVALID
+/// @brief The different legalization strategies the cluster legalizer can perform.
+///
+/// Allows the user of the API to select how thorough the legalizer should be.
+enum class ClusterLegalizationStrategy {
+    FULL,                   // Run the full legalizer (including intra-lb routing)
+    SKIP_INTRA_LB_ROUTE     // Do all legality checks except intra-lb routing
+};
+
+/// @brief The status of the cluster legalization.
+enum class e_block_pack_status {
+    BLK_PASSED,                 // Passed legalization.
+    BLK_FAILED_FEASIBLE,        // Failed due to block not feasibly being able to go in the cluster.
+    BLK_FAILED_ROUTE,           // Failed due to intra-lb routing failure.
+    BLK_FAILED_FLOORPLANNING,   // Failed due to not being compatible with the cluster's current PartitionRegion.
+    BLK_FAILED_NOC_GROUP,       // Failed due to not being compatible with the cluster's NoC group.
+    BLK_STATUS_UNDEFINED        // Undefined status. Something went wrong.
 };
 
 /*
@@ -49,25 +59,25 @@ struct LegalizationCluster {
     /// 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.
-    t_pb* cluster_pb;
+    t_pb* pb;
 
     /// @brief The logical block type this cluster represents.
-    t_logical_block_type_ptr cluster_type;
+    t_logical_block_type_ptr type;
 
     /// @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.
-    PartitionRegion cluster_pr;
+    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.
-    NocGroupId cluster_noc_grp_id;
+    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...
-    t_lb_router_data* cluster_router_data;
+    t_lb_router_data* router_data;
 };
 
 // FIXME: We should completely remove access to the atom context. The legalizer
@@ -82,19 +92,21 @@ class ClusterLegalizer {
                                           const t_ext_pin_util& max_external_pin_util);
 public:
 
+    // Default constructor. Use the init method to initialize the legalizer.
     ClusterLegalizer() = default;
 
     /*
      * @brief Initialize the ClusterLegalizer class. Must be called before use.
      *
      * Allocates internal state.
+     * FIXME: See if we can simplify the parameters. Can any of these be inferred?
      */
     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,
-              int t_detailed_routing_stage,
+              ClusterLegalizationStrategy t_cluster_legalization_strategy,
               bool t_enable_pin_feasibility_filter,
               int t_feasible_block_array_size,
               int t_log_verbosity,
@@ -147,27 +159,58 @@ class ClusterLegalizer {
         VTR_ASSERT(false && "Not implemented.");
     }
 
+    // TODO: It would be nice to have an API to remove a molecule from a cluster.
+
     // TODO: Make a better name for this. The basic idea is that it will uncluster
     //       all molecules in the cluster and free all aspects of the cluster.
     void destroy_cluster(LegalizationClusterId cluster_id);
 
     /*
-     * @brief Double check that the given cluster is legal.
+     * @brief Check that the given cluster is fully legal.
      *
      * This method runs an intra_lb_route on the given cluster. This ignores
-     * the detailed_routing_stage parameter to this class.
+     * the cluster legalization strategy set by the user. This method will not
+     * correct the problematic molecules, it will only return true if the
+     * cluster is legal and false if it is not.
+     *
+     *  @param cluster_id       The ID of the cluster to fully legalize.
+     *
+     *  @return                 True if the cluster is legal, false otherwise.
      */
     bool check_cluster_legality(LegalizationClusterId cluster_id);
 
-    inline const LegalizationCluster& get_cluster(LegalizationClusterId cluster_id) const {
+    inline t_pb* get_cluster_pb(LegalizationClusterId cluster_id) const {
         VTR_ASSERT_SAFE(cluster_id.is_valid() && (size_t)cluster_id < legalization_clusters.size());
-        return legalization_clusters[cluster_id];
+        LegalizationCluster cluster = legalization_clusters[cluster_id];
+        return cluster.pb;
     }
 
-    void inline set_detailed_routing_stage(int stage) {
-        detailed_routing_stage = stage;
+    /*
+     * @brief Set the legalization strategy of the cluster legalizer.
+     *
+     * This allows the strategy of the cluster legalizer to change based on the
+     * needs of the user. For example, one can set the legalizer to use a more
+     * relaxed strategy to insert a batch of molecules in cheaply, saving the
+     * full legalizerion for the end (using check_cluster_legality).
+     *
+     *  @param strategy     The strategy to set the cluster legalizer to.
+     */
+    void inline set_legalization_strategy(ClusterLegalizationStrategy strategy) {
+        cluster_legalization_strategy = strategy;
     }
 
+    /*
+     * @brief Set how verbose the log messages should be for the cluster legalizer.
+     *
+     * This allows the user to set the verbosity at different points for easier
+     * usability.
+     *
+     * Set the verbosity to 4 to see most of the log messages on how the molecules
+     * move through the legalizer.
+     * Set the verbosity to 5 to see all the log messages in the legalizer.
+     *
+     *  @param verbosity    The value to set the verbosity to.
+     */
     inline void set_log_verbosity(int verbosity) {
         log_verbosity = verbosity;
     }
@@ -176,15 +219,23 @@ class ClusterLegalizer {
 
     void reset();
 
+    /// @brief Destructor of the class. Calls the reset method.
     ~ClusterLegalizer() { reset(); }
 
     // TODO: This should be a vector.
+    // FIXME: Make this private and create a getter. This may need to be left
+    //        accessible.
+    // FIXME: Something fishy is going on with this. See update_cluster_stats.
+    //        Need to cross-reference and see what matters and what doesnt for
+    //        legalization. What variables do we touch and does this function
+    //        touch it!
     t_cluster_placement_stats* cluster_placement_stats = nullptr;
 
 private:
     /// @brief Lookup table for which cluster each molecule is in.
     std::unordered_map<t_pack_molecule*, LegalizationClusterId> molecule_cluster;
-    
+
+    /// @brief List of all legalization clusters.
     vtr::vector<LegalizationClusterId, LegalizationCluster> legalization_clusters;
 
     // FIXME: This can be made a vector if the atom netlist is passed into init.
@@ -205,8 +256,8 @@ class ClusterLegalizer {
 
     size_t num_models;
 
-    // TODO: Make this a better enum (enum class).
-    int detailed_routing_stage;
+    /// @brief The current legalization strategy of the cluster legalizer.
+    ClusterLegalizationStrategy cluster_legalization_strategy;
 
     bool enable_pin_feasibility_filter;
 
diff --git a/vpr/src/pack/cluster_util.cpp b/vpr/src/pack/cluster_util.cpp
@@ -300,31 +300,6 @@ void get_max_cluster_size_and_pb_depth(int& max_cluster_size,
     }
 }
 
-bool check_cluster_legality(const int& verbosity,
-                            const int& detailed_routing_stage,
-                            t_lb_router_data* router_data) {
-    bool is_cluster_legal;
-
-    if (detailed_routing_stage == (int)E_DETAILED_ROUTE_AT_END_ONLY) {
-        /* is_mode_conflict does not affect this stage. It is needed when trying to route the packed clusters.
-         *
-         * It holds a flag that is used to verify whether try_intra_lb_route ended in a mode conflict issue.
-         * If the value is TRUE the cluster has to be repacked, and its internal pb_graph_nodes will have more restrict choices
-         * for what regards the mode that has to be selected
-         */
-        t_mode_selection_status mode_status;
-        is_cluster_legal = try_intra_lb_route(router_data, verbosity, &mode_status);
-        if (is_cluster_legal) {
-            VTR_LOGV(verbosity > 2, "\tPassed route at end.\n");
-        } else {
-            VTR_LOGV(verbosity > 0, "Failed route at end, repack cluster trying detailed routing at each stage.\n");
-        }
-    } else {
-        is_cluster_legal = true;
-    }
-    return is_cluster_legal;
-}
-
 /*print the header for the clustering progress table*/
 void print_pack_status_header() {
     VTR_LOG("Starting Clustering - Clustering Progress: \n");
@@ -1402,12 +1377,12 @@ void start_new_cluster(ClusterLegalizer& cluster_legalizer,
             //        - Also this code may be able to partially be put in the
             //          legalizer
             //        - also the control flow of this function is really bad.
-            const LegalizationCluster& legalization_cluster = cluster_legalizer.get_cluster(new_cluster_id);
-            if (legalization_cluster.cluster_pb->name != nullptr) {
-                free(legalization_cluster.cluster_pb->name);
+            t_pb* cluster_pb = cluster_legalizer.get_cluster_pb(new_cluster_id);
+            if (cluster_pb->name != nullptr) {
+                free(cluster_pb->name);
             }
-            legalization_cluster.cluster_pb->name = vtr::strdup(root_atom_name.c_str());
-            clb_index = clb_nlist->create_block(root_atom_name.c_str(), legalization_cluster.cluster_pb, type);
+            cluster_pb->name = vtr::strdup(root_atom_name.c_str());
+            clb_index = clb_nlist->create_block(root_atom_name.c_str(), cluster_pb, type);
             legalization_cluster_id = new_cluster_id;
             // FIXME: This was stolen out of the legalizer. Need to be made better!
             // - Also it may be wrong. See try_place_atom_block_rec
diff --git a/vpr/src/pack/cluster_util.h b/vpr/src/pack/cluster_util.h
@@ -122,10 +122,6 @@ void check_and_output_clustering(const t_packer_opts& packer_opts,
 void get_max_cluster_size_and_pb_depth(int& max_cluster_size,
                                        int& max_pb_depth);
 
-bool check_cluster_legality(const int& verbosity,
-                            const int& detailed_routing_stage,
-                            t_lb_router_data* router_data);
-
 bool is_atom_blk_in_pb(const AtomBlockId blk_id, const t_pb* pb);
 
 void add_molecule_to_pb_stats_candidates(t_pack_molecule* molecule,
diff --git a/vpr/src/pack/re_cluster_util.cpp b/vpr/src/pack/re_cluster_util.cpp
@@ -121,7 +121,7 @@ bool start_new_cluster_for_mol(t_pack_molecule* molecule,
                                t_lb_router_data** router_data,
                                PartitionRegion& temp_cluster_pr,
                                NocGroupId& temp_cluster_noc_grp_id,
-                               enum e_detailed_routing_stages detailed_routing_stage,
+                               // enum e_detailed_routing_stages detailed_routing_stage,
                                int force_site) {
     auto& atom_ctx = g_vpr_ctx.atom();
     auto& floorplanning_ctx = g_vpr_ctx.mutable_floorplanning();
@@ -160,7 +160,7 @@ bool start_new_cluster_for_mol(t_pack_molecule* molecule,
                                     helper_ctx.num_models,
                                     helper_ctx.max_cluster_size,
                                     clb_index,
-                                    detailed_routing_stage,
+                                    0/*detailed_routing_stage*/,
                                     *router_data,
                                     0,
                                     enable_pin_feasibility_filter,
@@ -218,7 +218,7 @@ bool pack_mol_in_existing_cluster(t_pack_molecule* molecule,
                                   bool during_packing,
                                   t_clustering_data& clustering_data,
                                   t_lb_router_data*& router_data,
-                                  enum e_detailed_routing_stages detailed_routing_stage,
+                                  // enum e_detailed_routing_stages detailed_routing_stage,
                                   bool enable_pin_feasibility_filter,
                                   int force_site) {
 
@@ -247,7 +247,7 @@ bool pack_mol_in_existing_cluster(t_pack_molecule* molecule,
                                     helper_ctx.num_models,
                                     helper_ctx.max_cluster_size,
                                     new_clb,
-                                    detailed_routing_stage,
+                                    0/*detailed_routing_stage*/,
                                     router_data,
                                     0,
                                     enable_pin_feasibility_filter,
@@ -310,7 +310,7 @@ void revert_mol_move(ClusterBlockId old_clb,
                                                         helper_ctx.num_models,
                                                         helper_ctx.max_cluster_size,
                                                         old_clb,
-                                                        E_DETAILED_ROUTE_FOR_EACH_ATOM,
+                                                        0/*(int)e_detailed_routing_stages::E_DETAILED_ROUTE_FOR_EACH_ATOM*/,
                                                         old_router_data,
                                                         0,
                                                         helper_ctx.enable_pin_feasibility_filter,
@@ -666,8 +666,10 @@ static void rebuild_cluster_placement_stats(ClusterBlockId clb_index,
     }
 }
 
-bool is_cluster_legal(t_lb_router_data*& router_data) {
-    return (check_cluster_legality(0, E_DETAILED_ROUTE_AT_END_ONLY, router_data));
+bool is_cluster_legal(t_lb_router_data*& /*router_data*/) {
+    // FIXME: This should use the new API.
+    // return (check_cluster_legality(0, (int)e_detailed_routing_stages::E_DETAILED_ROUTE_AT_END_ONLY, router_data));
+    return true;
 }
 
 void commit_mol_removal(const t_pack_molecule* molecule,
diff --git a/vpr/src/pack/re_cluster_util.h b/vpr/src/pack/re_cluster_util.h
