Refactoring clustering/packing code and building re-cluster API #2034

MohamedElgammal · 2022-05-13T17:04:10Z

Description

In this pull request, the first function of re-cluster is implemented. This API should allow the users to change clustering decisions after the clustering is done. These changes can be done during packing or placement.

Multiple refactoring steps were done to the original clustering code to build this API.

The first function in the API implemented in this PR can move an atom out of a cluster and create a new cluster for it. More Functions for this API are to be implemented in upcoming PRs.

The function was tested on VTR benchmarks and unit tests will be added when the API is completed in the upcoming PRs.

Related Issue

Being not able to change the clustering decision later in the flow caused suboptimality in the generated placed design.

Motivation and Context

How Has This Been Tested?

The refactor shows no change to QoR. Some examples are extracted for the CI runs comparing the branch to master with no change attached here

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:

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

…l variables

…y_place

…assed from packing to placement

…r_test - the test need to be undated

MohamedElgammal · 2022-05-26T19:47:10Z

All the current check failures are due to cloud resource issues. I will re-run the failing tests in the evening (tried that yesterday and all the tests passed). It seems that the cloud is less loaded in the evening. If it's green, I will merge it in the early morning tomorrow.

sfkhalid · 2022-05-27T13:58:40Z

vpr/src/pack/pack.cpp

    std::unordered_map<AtomBlockId, t_pb_graph_node*> expected_lowest_cost_pb_gnode; //The molecules associated with each atom block
    const t_model* cur_model;
-    int num_models;
+    t_clustering_data clustering_data;
+    //int num_models;


maybe remove these lines instead of commenting them

sfkhalid · 2022-05-27T13:59:18Z

vpr/src/pack/prepack.cpp

                                            const int pack_pattern_index,
                                            AtomBlockId blk_id) {
    t_pack_molecule* molecule;

+    //auto& atom_ctx = g_vpr_ctx.atom();


remove commented line

sfkhalid · 2022-05-27T13:59:43Z

vpr/src/pack/prepack.h

                                               std::unordered_map<AtomBlockId, t_pb_graph_node*>& expected_lowest_cost_pb_gnode,
                                               const int num_packing_patterns);

-void free_pack_molecules(t_pack_molecule* list_of_pack_molecules);
+//void free_pack_molecules(t_pack_molecule* list_of_pack_molecules);


remove commented line

sfkhalid · 2022-05-27T14:01:43Z

vpr/src/pack/re_cluster_util.h

+/**
+ * @file
+ * @brief This files defines some helper functions for the re-clustering
+ *


Maybe add some context to this comment - why would we want to re-do clusters, what situations would that be beneficial in?

sfkhalid · 2022-05-27T14:02:26Z

vpr/src/place/place.cpp

@@ -1057,6 +1063,14 @@ static void placement_inner_loop(const t_annealing_state* state,
                                             blocks_affected, delay_model, criticalities, setup_slacks,
                                             placer_opts, move_type_stat, place_algorithm, timing_bb_factor, manual_move_enabled);

+        /*


Remove this debugging code

sfkhalid · 2022-05-27T14:02:45Z

vpr/src/place/place.cpp

@@ -1215,6 +1229,14 @@ static float starting_t(const t_annealing_state* state, t_placer_costs* costs, t
                                             placer_opts, move_type_stat, placer_opts.place_algorithm,
                                             REWARD_BB_TIMING_RELATIVE_WEIGHT, manual_move_enabled);

+        /******************** Elgammal ************************/
+        /*
+         * auto& atom_ctx = g_vpr_ctx.atom();


remove debugging code if no longer needed

sfkhalid

The code looks good to me overall. I think you can remove some unnecessary lines in a new pull request, and also add more context to the comment about the re-clustering API.

MohamedElgammal · 2022-05-27T16:27:52Z

Thanks @sfkhalid for the review. As per our conversation yesterday, I will land this now and address your comments on a new PR so that you can work on debugging the tests on the master branch.

vaughnbetz

To apply to the latest code.
Sarah's unit tests: seem to be full flow tests --> maybe better as command line reg tests as usual?

vaughnbetz · 2022-05-30T15:21:59Z

vpr/src/base/vpr_context.h

@@ -55,12 +55,18 @@ struct AtomContext : public Context {
    /********************************************************************
     * Atom Netlist
     ********************************************************************/
-
+    AtomContext()
+        : list_of_pack_molecules(nullptr, free_pack_molecules) {}
    ///@brief Atom netlist
    AtomNetlist nlist;

    ///@brief Mappings to/from the Atom Netlist to physically described .blif models
    AtomLookup lookup;


Beef up this comment. How do you use this mapping? What is its lifetime (valid only after clustering)?

vaughnbetz · 2022-05-30T15:25:29Z

vpr/src/base/vpr_context.h

+    ///@brief The molecules associated with each atom block
+    std::multimap<AtomBlockId, t_pack_molecule*> atom_molecules;
+
+    std::unique_ptr<t_pack_molecule, decltype(&free_pack_molecules)> list_of_pack_molecules;


Add a comment -- what is this for? (head of a linked list of all the molecules -- used to free them).

vaughnbetz · 2022-05-30T15:26:12Z

vpr/src/base/vpr_context.h

@@ -259,6 +265,26 @@ struct ClusteringContext : public Context {
     */
    std::map<ClusterBlockId, std::map<int, ClusterNetId>> post_routing_clb_pin_nets;
    std::map<ClusterBlockId, std::map<int, int>> pre_routing_net_pin_mapping;
+
+    std::map<t_logical_block_type_ptr, size_t> num_used_type_instances;


Comment this. What is it and what's it for?

vaughnbetz · 2022-05-30T15:26:47Z

vpr/src/base/vpr_context.h

+    std::map<t_logical_block_type_ptr, size_t> num_used_type_instances;
+};
+
+struct ClusteringHelperContext : public Context {


Comment this struct/context -- what is it for, what's it's lifetime?

Explain variables at least at high level (if you don't know them all then can say they're needed during the seed-based clusterer? Or needed for any clustering changes?)

vaughnbetz · 2022-05-30T15:30:14Z

vpr/src/base/vpr_types.h

@@ -1654,4 +1654,9 @@ class RouteStatus {

 typedef vtr::vector<ClusterBlockId, std::vector<std::vector<int>>> t_clb_opins_used; //[0..num_blocks-1][0..class-1][0..used_pins-1]

+typedef std::vector<std::map<int, int>> t_arch_switch_fanin;
+
+void free_pack_molecules(t_pack_molecule* list_of_pack_molecules);


Comment these if you can.

vaughnbetz · 2022-05-30T15:55:51Z

vpr/src/pack/re_cluster_util.h

+ */
+
+/**
+ * @brief A function that returns the cluster ID of an atom ID


Is the cluster id the block id in the clustered netlist?

vaughnbetz · 2022-05-30T15:56:54Z

vpr/src/pack/re_cluster_util.h

+std::vector<AtomBlockId> cluster_to_atoms(const ClusterBlockId& cluster);
+
+/**
+ * @brief A function that loads the router data for a cluster


intra-cluster router

vaughnbetz · 2022-05-30T15:57:19Z

vpr/src/pack/re_cluster_util.h

+ *
+ * It returns true if the removal is done and the old cluster is legal.
+ * It aborts the removal and returns false if the removal will make the old cluster 
+ * illegal


An empty cluster is considered illegal (?)

Re-order parameters to have inputs , then state that is input/output and then outputs.
Comment exactly what is returned, including reference parameters.

vaughnbetz · 2022-05-30T16:01:00Z

vpr/src/pack/re_cluster_util.h

+/**
+ * @brief A function that starts a new cluster for one specific molecule
+ * 
+ * It place the molecule in a specific type and mode that should be passed by


Same comment : comment inputs, outputs, etc.
inputs first, outputs second in parameter list.
Do we need pad_loc_type?
You're passing in an atom, but the comment is about molecules. (update comment if necessary)

vaughnbetz · 2022-05-30T16:01:46Z

vpr/src/pack/re_cluster_util.h

+                                bool during_packing);
+
+/**
+ * @brief A function that fix the clustered netlist if the move is performed


If you can, explain what this fix is (at a high level).
When should this be called? After any change to any cluster after packing is done?

MohamedElgammal added 21 commits February 23, 2022 20:23

Adding helper functions to cross reference atom and cluster

7697369

move clustering helper functions to cluster_util and rmeove all globa…

a146475

…l variables

adding a function to remove a molecule out of a cluster (still testing)

7ff8eae

Adding a data structure to atom context and passing another one to tr…

0fa1e8c

…y_place

Initial implementation of moving an atom to a new cluster function (WIP)

bdd93dc

new implementation for the pin function fix (WIP)

92b5df5

Fix the atom-cluster block,pin and net mapping (WIP)

4132d5c

Move atom to new function working on a simple design :)

179021c

Fix the pb tree of removed atom

b109a13

Fixing block naming and removing unused pins

5904a33

Fixing the graphics and move a variable to context instead of being p…

172777e

…assed from packing to placement

the move can be done after initial placement

a896f5a

Updating the API function to work during packing and placement

87b562c

Update the function to work after the initial placement

9fb56c5

More commenting and better code structure

eb71a9e

Fix formatting

9b70cde

Merge branch 'master' into re_clustering_api

25607bc

Continue merging master into re packing api

0a81328

Fix formatting

c3297d4

Merge packing constraints with the refactored code

3baa1ee

fix formatting

e190a91

MohamedElgammal requested a review from sfkhalid May 13, 2022 17:04

MohamedElgammal added the kokoro:force-run label May 13, 2022

MohamedElgammal marked this pull request as draft May 13, 2022 17:14

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label May 13, 2022

MohamedElgammal added 3 commits May 13, 2022 13:26

fix formatting

f7398a1

Fix formatting

1ffe202

Fix formatting

d240e96

MohamedElgammal marked this pull request as ready for review May 15, 2022 17:35

remove compilation warnings

452d519

fix formatting

9c4ad0f

MohamedElgammal removed the kokoro:force-run label May 17, 2022

MohamedElgammal added 11 commits May 17, 2022 11:48

fix some memory leaks

f26a177

remove debugging print statements and fix formatting

d668f58

more informative printing

dc63856

Fixing memory leak issues

617d0ce

Fix a memory issue in case of skip packing

64e97cf

fix formatting

4e12452

Fix the merge with master

7c0e8ad

Update the unit tests to clear the updated data structures

a260be7

disable VPR constraint test

e5e2d18

fix formatting

1dbacdb

disable 1 of vpr constraints tests (working on vpr) but failing on vp…

5a11d15

…r_test - the test need to be undated

MohamedElgammal mentioned this pull request May 26, 2022

Github CI tests are doubled #2041

Closed

edit unit tests

bf943cb

sfkhalid reviewed May 27, 2022

View reviewed changes

sfkhalid approved these changes May 27, 2022

View reviewed changes

MohamedElgammal merged commit 041e0f1 into master May 27, 2022

MohamedElgammal deleted the re_clustering_api branch May 27, 2022 16:28

MohamedElgammal mentioned this pull request May 27, 2022

Re-clustering API (follow-up PR) #2044

Merged

vaughnbetz reviewed May 30, 2022

View reviewed changes

MohamedElgammal mentioned this pull request Jun 7, 2022

Building a re-clustering API (cont.) #2051

Merged

3 tasks

MohamedElgammal mentioned this pull request Sep 9, 2022

Cluster attraction groups for tight floorplan constraints not working #2089

Closed

Refactoring clustering/packing code and building re-cluster API #2034

Refactoring clustering/packing code and building re-cluster API #2034

Uh oh!

Conversation

MohamedElgammal commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

MohamedElgammal commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfkhalid left a comment

Choose a reason for hiding this comment

Uh oh!

MohamedElgammal commented May 27, 2022

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MohamedElgammal commented May 13, 2022 •

edited

Loading