Improving Unrelated Clustering Feature in VPR #2237

kimiatkh · 2023-02-03T23:27:30Z

Description

In this pull request, we aim to improve the unrelated clustering feature in VPR. The current feature allows the packer to group unrelated molecules together, but it applies to all block types indiscriminately. We address this limitation by allowing users to set the feature separately for each block type.

Additionally, when the packer fails to pack the design densely enough to fit on the device, it currently turns on unrelated clustering for all block types, which can lead to worsened results for block types that were already fit. To resolve this issue, we have enabled the packer to turn on unrelated clustering only for block types whose utilization exceeds a certain threshold.

Motivation and Context

This improvement was motivated by the results of running the Titan23 benchmarks on a Stratix 10 device model with a fixed layout. Many IO-dense designs experienced congestion and routing failures due to the packer's indiscriminate use of the unrelated clustering feature. When the packer was unable to pack the IO pins densely enough, the unrelated clustering feature was turned on for all block types, including LABs, which led to increased congestion in the routing.

How Has This Been Tested?

The improved feature has been tested by using it on the designs that failed in the Titan23 benchmarks.

Types of changes

[] Bug fix (change which fixes an issue)
[ x] New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ x] My change requires a change to the documentation
[ x] I have updated the documentation accordingly
I have added tests to cover my changes
[ x] All new and existing tests passed

vaughnbetz · 2023-02-06T22:35:31Z

vpr/src/base/vpr_types.h

@@ -133,10 +133,14 @@ enum class e_const_gen_inference {
    COMB_SEQ ///<Both combinational and sequential constant generator inference
 };

-enum class e_unrelated_clustering {
+enum class e_unrel_clust_stat {


comment usage / meaning

vaughnbetz · 2023-02-06T22:37:26Z

vpr/src/base/vpr_types.h

+     * The unrelated clustering status is represented as a pair of booleans. 
+     * The first boolean incidcates whether unrelated clustering is turned on or off. 
+     * The second variable indicates whether the status of the variable is provided
+     * by the user(bool = false) or user let it to be automatically determined by VPR(bool = ture). 


ture -> true

Using @param in this description and being more precise (not pair of booleans; instead it is a pair of status control variables) would be good here

vaughnbetz · 2023-02-06T22:41:02Z

vpr/src/base/vpr_types.h

+    void set_default_status(std::pair<enum e_unrel_clust_stat, enum e_unrel_clust_mode> status);
+
+  private:
+    std::pair<enum e_unrel_clust_stat, enum e_unrel_clust_mode> default_;


Comment what these are: default value for all block types, and overrides by block_type name for specific types. Each says whether unrelated cluster is on or off, and if it was set to that state by the user or automatic vpr behaviour.

vaughnbetz · 2023-02-06T22:42:45Z

vpr/src/pack/pack.cpp

@@ -27,11 +27,15 @@
 /* #define DUMP_PB_GRAPH 1 */
 /* #define DUMP_BLIF_INPUT 1 */

-static bool try_size_device_grid(const t_arch& arch, const std::map<t_logical_block_type_ptr, size_t>& num_type_instances, float target_device_utilization, std::string device_layout_name);
+static std::vector<std::string> try_size_device_grid(const t_arch& arch, const std::map<t_logical_block_type_ptr, size_t>& num_type_instances, float target_device_utilization, std::string device_layout_name);


Comment what this returns (not a super obvious return type). If you know the meaning of the other variables and what the routine does comment that too at the same time.

vaughnbetz · 2023-02-06T22:43:08Z

vpr/src/pack/pack.cpp


 static t_ext_pin_util_targets parse_target_external_pin_util(std::vector<std::string> specs);
 static std::string target_external_pin_util_to_string(const t_ext_pin_util_targets& ext_pin_utils);

+static t_allow_unrelated_clustering parse_unrelated_clustering_stat(std::vector<std::string> specs);


Add comments.

vaughnbetz · 2023-02-06T22:45:59Z

vpr/src/pack/pack.cpp

+            if (fixed_blocks.size() > 0) {
+                std::stringstream msg;
+                msg << "Packing failed to fit on device.\n"
+                    << "The following block types were overused, but unrelated clustering was disabled for them:\n"


Is unrelated clustering set for them, or just a fixed mode (either on or off)? I think this message isn't quite right.

vaughnbetz · 2023-02-06T22:48:25Z

vpr/src/pack/pack.cpp


        /* We use this bool to determine the cause for the clustering not being dense enough. If the clustering
         * is not dense enough and there are floorplan constraints, it is presumed that the constraints are the cause
         * of the floorplan not fitting, so attraction groups are turned on for later iterations.
         */
        bool floorplan_not_fitting = (floorplan_regions_overfull || g_vpr_ctx.mutable_floorplanning().constraints.get_num_partitions() > 0);

-        if (fits_on_device && !floorplan_regions_overfull) {
+        if (overused_blocks.empty() && !floorplan_regions_overfull) {
            break; //Done
        } else if (pack_iteration == 1 && !floorplan_not_fitting) {
            //1st pack attempt was unsucessful (i.e. not dense enough) and we have control of unrelated clustering
            //
            //Turn it on to increase packing density


This is a long subroutine, so it would be better to pull this auto-packing code all new code + some of the packer_opts->balance_block_type_utilization code) into a new helper function.

vaughnbetz · 2023-02-06T22:48:59Z

vpr/src/pack/pack.cpp

@@ -342,7 +356,7 @@ std::unordered_set<AtomNetId> alloc_and_load_is_clock(bool global_clocks) {
    return (is_clock);
 }

-static bool try_size_device_grid(const t_arch& arch, const std::map<t_logical_block_type_ptr, size_t>& num_type_instances, float target_device_utilization, std::string device_layout_name) {
+static std::vector<std::string> try_size_device_grid(const t_arch& arch, const std::map<t_logical_block_type_ptr, size_t>& num_type_instances, float target_device_utilization, std::string device_layout_name) {


Make sure there is a comment on the earlier declaration of this function.

vaughnbetz · 2023-02-06T22:50:23Z

vpr/src/pack/pack.cpp

+    }
+    return conv_value;
+}
+static bool block_type_found(std::string block_type_name) {


Add vertical space.

Ensure there is a comment on the earlier function declaration.

vaughnbetz · 2023-02-17T23:57:24Z

Looks good. Shouldn't affect QoR, but please check somehow (test QoR or confirm results don't change at all on a few circuits).

vaughnbetz · 2023-09-25T22:41:18Z

@kimiatkh : please resolve the conflicts and we can merge this. Please check at least one design gets identical QoR before and after the change.

vaughnbetz · 2024-05-23T18:38:16Z

@kimiatkh : it would be good to land this ...

kimiatkh added 3 commits February 1, 2023 19:46

Enabling unrelated clustering by block type

826cc37

make format

6034bf6

Updating the documentation

919f1df

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Feb 3, 2023

kimiatkh requested a review from vaughnbetz February 3, 2023 23:27

vaughnbetz requested changes Feb 6, 2023

View reviewed changes

kimiatkh added 5 commits February 10, 2023 19:15

Add more comments

55c929b

Refactor the try_pack function

a29adb2

Add comments

e3893a5

Add comments

91884fe

make foramt

7500fcd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving Unrelated Clustering Feature in VPR #2237

Improving Unrelated Clustering Feature in VPR #2237

kimiatkh commented Feb 3, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz Feb 6, 2023

vaughnbetz commented Feb 17, 2023

vaughnbetz commented Sep 25, 2023

vaughnbetz commented May 23, 2024

Improving Unrelated Clustering Feature in VPR #2237

Are you sure you want to change the base?

Improving Unrelated Clustering Feature in VPR #2237

Conversation

kimiatkh commented Feb 3, 2023

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaughnbetz commented Feb 17, 2023

vaughnbetz commented Sep 25, 2023

vaughnbetz commented May 23, 2024