[vpr][place] addressing PR comments

Author: Yulang Luo

vpr/src/place/initial_noc_placement.cpp

@@ -220,7 +220,7 @@ static void noc_routers_anneal(const t_noc_opts& noc_opts) {
     // Generate and evaluate router moves
     for (int i_move = 0; i_move < N_MOVES; i_move++) {
         e_create_move create_move_outcome = e_create_move::ABORT;
-        clear_move_blocks(blocks_affected);
+        blocks_affected.clear_move_blocks();
         // Shrink the range limit over time
         float r_lim_decayed = 1.0f + (N_MOVES - i_move) * (max_r_lim / N_MOVES);
         create_move_outcome = propose_router_swap(blocks_affected, r_lim_decayed);

vpr/src/place/move_transactions.cpp

@@ -1,3 +1,4 @@
+#include "move_transactions.h"
 #include "move_utils.h"
 
 #include "globals.h"
@@ -6,7 +7,6 @@
 
 t_pl_blocks_to_be_moved::t_pl_blocks_to_be_moved(size_t max_blocks){
         moved_blocks.reserve(max_blocks);
-        moved_blocks.resize(0);
 }
 
 size_t t_pl_blocks_to_be_moved::get_size_and_increment() {
@@ -16,9 +16,9 @@ size_t t_pl_blocks_to_be_moved::get_size_and_increment() {
 }
 
 //Records that block 'blk' should be moved to the specified 'to' location
-e_block_move_result record_block_move(t_pl_blocks_to_be_moved& blocks_affected, ClusterBlockId blk, t_pl_loc to) {
-    auto res = blocks_affected.moved_to.emplace(to);
-    if (!res.second) {
+e_block_move_result t_pl_blocks_to_be_moved::record_block_move(ClusterBlockId blk, t_pl_loc to) {
+    auto [to_it, to_success] = moved_to.emplace(to);
+    if (!to_success) {
         log_move_abort("duplicate block move to location");
         return e_block_move_result::ABORT;
     }
@@ -27,35 +27,57 @@ e_block_move_result record_block_move(t_pl_blocks_to_be_moved& blocks_affected,
 
     t_pl_loc from = place_ctx.block_locs[blk].loc;
 
-    auto res2 = blocks_affected.moved_from.emplace(from);
-    if (!res2.second) {
+    auto [_, from_success] = moved_from.emplace(from);
+    if (!from_success) {
+        moved_to.erase(to_it);
         log_move_abort("duplicate block move from location");
         return e_block_move_result::ABORT;
     }
 
     VTR_ASSERT_SAFE(to.sub_tile < int(place_ctx.grid_blocks.num_blocks_at_location({to.x, to.y, to.layer})));
 
     // Sets up the blocks moved
-    size_t imoved_blk = blocks_affected.get_size_and_increment();
-    blocks_affected.moved_blocks[imoved_blk].block_num = blk;
-    blocks_affected.moved_blocks[imoved_blk].old_loc = from;
-    blocks_affected.moved_blocks[imoved_blk].new_loc = to;
+    size_t imoved_blk = get_size_and_increment();
+    moved_blocks[imoved_blk].block_num = blk;
+    moved_blocks[imoved_blk].old_loc = from;
+    moved_blocks[imoved_blk].new_loc = to;
 
     return e_block_move_result::VALID;
 }
 
+//Examines the currently proposed move and determine any empty locations
+std::set<t_pl_loc> t_pl_blocks_to_be_moved::t_pl_blocks_to_be_moved::determine_locations_emptied_by_move() {
+    std::set<t_pl_loc> moved_from_set;
+    std::set<t_pl_loc> moved_to_set;
+
+    for (const t_pl_moved_block& moved_block : moved_blocks) {
+        //When a block is moved its old location becomes free
+        moved_from_set.emplace(moved_block.old_loc);
+
+        //But any block later moved to a position fills it
+        moved_to_set.emplace(moved_block.new_loc);
+    }
+
+    std::set<t_pl_loc> empty_locs;
+    std::set_difference(moved_from_set.begin(), moved_from_set.end(),
+                        moved_to_set.begin(), moved_to_set.end(),
+                        std::inserter(empty_locs, empty_locs.begin()));
+
+    return empty_locs;
+}
+
 //Moves the blocks in blocks_affected to their new locations
 void apply_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {
     auto& place_ctx = g_vpr_ctx.mutable_placement();
     auto& device_ctx = g_vpr_ctx.device();
 
     //Swap the blocks, but don't swap the nets or update place_ctx.grid_blocks
     //yet since we don't know whether the swap will be accepted
-    for (const auto& block : blocks_affected.moved_blocks) {
-        ClusterBlockId blk = block.block_num;
+    for (const t_pl_moved_block& moved_block : blocks_affected.moved_blocks) {
+        ClusterBlockId blk = moved_block.block_num;
 
-        const t_pl_loc& old_loc = block.old_loc;
-        const t_pl_loc& new_loc = block.new_loc;
+        const t_pl_loc& old_loc = moved_block.old_loc;
+        const t_pl_loc& new_loc = moved_block.new_loc;
 
         // move the block to its new location
         place_ctx.block_locs[blk].loc = new_loc;
@@ -78,11 +100,11 @@ void commit_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {
     auto& place_ctx = g_vpr_ctx.mutable_placement();
 
     /* Swap physical location */
-    for (const auto& block : blocks_affected.moved_blocks) {
-        ClusterBlockId blk = block.block_num;
+    for (const t_pl_moved_block& moved_block : blocks_affected.moved_blocks) {
+        ClusterBlockId blk = moved_block.block_num;
 
-        const t_pl_loc& to = block.new_loc;
-        const t_pl_loc& from = block.old_loc;
+        const t_pl_loc& to = moved_block.new_loc;
+        const t_pl_loc& from = moved_block.old_loc;
 
         //Remove from old location only if it hasn't already been updated by a previous block update
         if (place_ctx.grid_blocks.block_at_location(from) == blk) {
@@ -108,11 +130,11 @@ void revert_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {
     auto& device_ctx = g_vpr_ctx.device();
 
     // Swap the blocks back, nets not yet swapped they don't need to be changed
-    for (const auto& block : blocks_affected.moved_blocks) {
-        ClusterBlockId blk = block.block_num;
+    for (const t_pl_moved_block& moved_block : blocks_affected.moved_blocks) {
+        ClusterBlockId blk = moved_block.block_num;
 
-        const t_pl_loc& old_loc = block.old_loc;
-        const t_pl_loc& new_loc = block.new_loc;
+        const t_pl_loc& old_loc = moved_block.old_loc;
+        const t_pl_loc& new_loc = moved_block.new_loc;
 
         // return the block to where it was before the swap
         place_ctx.block_locs[blk].loc = old_loc;
@@ -132,15 +154,15 @@ void revert_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {
 }
 
 //Clears the current move so a new move can be proposed
-void clear_move_blocks(t_pl_blocks_to_be_moved& blocks_affected) {
+void t_pl_blocks_to_be_moved::clear_move_blocks() {
     //Reset moved flags
-    blocks_affected.moved_to.clear();
-    blocks_affected.moved_from.clear();
+    moved_to.clear();
+    moved_from.clear();
 
     //For run-time, we just reset size of blocks_affected.moved_blocks to zero, but do not free the blocks_affected
     //array to avoid memory allocation
 
-    blocks_affected.moved_blocks.resize(0);
+    moved_blocks.resize(0);
 
-    blocks_affected.affected_pins.clear();
+    affected_pins.clear();
 }

vpr/src/place/move_transactions.h

@@ -3,6 +3,13 @@
 #include "vpr_types.h"
 #include "clustered_netlist_utils.h"
 
+enum class e_block_move_result {
+    VALID,       //Move successful
+    ABORT,       //Unable to perform move
+    INVERT,      //Try move again but with from/to inverted
+    INVERT_VALID //Completed inverted move
+};
+
 /* Stores the information of the move for a block that is       *
  * moved during placement                                       *
  * block_num: the index of the moved block                      *
@@ -33,30 +40,37 @@ struct t_pl_moved_block {
  *                graph.                                        */
 struct t_pl_blocks_to_be_moved {
     explicit t_pl_blocks_to_be_moved(size_t max_blocks);
+    t_pl_blocks_to_be_moved() = delete;
+    t_pl_blocks_to_be_moved(const t_pl_blocks_to_be_moved&) = delete;
+    t_pl_blocks_to_be_moved(t_pl_blocks_to_be_moved&&) = delete;
+
+/**
+ * @brief This function increments the size of the moved_blocks vector and return the index
+ * of the newly added last elements. 
+ */
+    size_t get_size_and_increment();
+
+/**
+ * @brief This function clears all data structures of this struct.
+ */
+    void clear_move_blocks();
+
+    e_block_move_result record_block_move(ClusterBlockId blk, t_pl_loc to);
+    
+    std::set<t_pl_loc> determine_locations_emptied_by_move();
 
     std::vector<t_pl_moved_block> moved_blocks;
     std::unordered_set<t_pl_loc> moved_from;
     std::unordered_set<t_pl_loc> moved_to;
 
     std::vector<ClusterPinId> affected_pins;
-    size_t get_size_and_increment();
-};
-
-enum class e_block_move_result {
-    VALID,       //Move successful
-    ABORT,       //Unable to perform move
-    INVERT,      //Try move again but with from/to inverted
-    INVERT_VALID //Completed inverted move
 };
 
-e_block_move_result record_block_move(t_pl_blocks_to_be_moved& blocks_affected, ClusterBlockId blk, t_pl_loc to);
 
 void apply_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected);
 
 void commit_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected);
 
 void revert_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected);
 
-void clear_move_blocks(t_pl_blocks_to_be_moved& blocks_affected);
-
 #endif

vpr/src/place/move_utils.cpp

@@ -1,5 +1,6 @@
 #include "move_utils.h"
 
+#include "move_transactions.h"
 #include "place_util.h"
 #include "globals.h"
 
@@ -141,7 +142,7 @@ e_block_move_result record_single_block_swap(t_pl_blocks_to_be_moved& blocks_aff
     // Check whether the to_location is empty
     if (b_to == EMPTY_BLOCK_ID) {
         // Sets up the blocks moved
-        outcome = record_block_move(blocks_affected, b_from, to);
+        outcome = blocks_affected.record_block_move(b_from, to);
 
     } else if (b_to != INVALID_BLOCK_ID) {
         // Check whether block to is compatible with from location
@@ -152,14 +153,14 @@ e_block_move_result record_single_block_swap(t_pl_blocks_to_be_moved& blocks_aff
         }
 
         // Sets up the blocks moved
-        outcome = record_block_move(blocks_affected, b_from, to);
+        outcome = blocks_affected.record_block_move(b_from, to);
 
         if (outcome != e_block_move_result::VALID) {
             return outcome;
         }
 
         t_pl_loc from = place_ctx.block_locs[b_from].loc;
-        outcome = record_block_move(blocks_affected, b_to, from);
+        outcome = blocks_affected.record_block_move(b_to, from);
 
     } // Finish swapping the blocks and setting up blocks_affected
 
@@ -341,7 +342,7 @@ e_block_move_result record_macro_move(t_pl_blocks_to_be_moved& blocks_affected,
 
         ClusterBlockId blk_to = place_ctx.grid_blocks.block_at_location(to);
 
-        record_block_move(blocks_affected, member.blk_index, to);
+        blocks_affected.record_block_move(member.blk_index, to);
 
         int imacro_to = -1;
         get_imacro_from_iblk(&imacro_to, blk_to, place_ctx.pl_macros);
@@ -392,7 +393,7 @@ e_block_move_result record_macro_self_swaps(t_pl_blocks_to_be_moved& blocks_affe
     auto& place_ctx = g_vpr_ctx.placement();
 
     //Reset any partial move
-    clear_move_blocks(blocks_affected);
+    blocks_affected.clear_move_blocks();
 
     //Collect the macros affected
     std::vector<int> affected_macros;
@@ -431,14 +432,14 @@ e_block_move_result record_macro_self_swaps(t_pl_blocks_to_be_moved& blocks_affe
     std::copy_if(displaced_blocks.begin(), displaced_blocks.end(), std::back_inserter(non_macro_displaced_blocks), is_non_macro_block);
 
     //Based on the currently queued block moves, find the empty 'holes' left behind
-    auto empty_locs = determine_locations_emptied_by_move(blocks_affected);
+    auto empty_locs = blocks_affected.determine_locations_emptied_by_move();
 
     VTR_ASSERT_SAFE(empty_locs.size() >= non_macro_displaced_blocks.size());
 
     //Fit the displaced blocks into the empty locations
     auto loc_itr = empty_locs.begin();
     for (auto blk : non_macro_displaced_blocks) {
-        outcome = record_block_move(blocks_affected, blk, *loc_itr);
+        outcome = blocks_affected.record_block_move(blk, *loc_itr);
         ++loc_itr;
     }
 
@@ -483,27 +484,6 @@ bool is_legal_swap_to_location(ClusterBlockId blk, t_pl_loc to) {
     return true;
 }
 
-//Examines the currently proposed move and determine any empty locations
-std::set<t_pl_loc> determine_locations_emptied_by_move(t_pl_blocks_to_be_moved& blocks_affected) {
-    std::set<t_pl_loc> moved_from;
-    std::set<t_pl_loc> moved_to;
-
-    for (const auto& block : blocks_affected.moved_blocks) {
-        //When a block is moved its old location becomes free
-        moved_from.emplace(block.old_loc);
-
-        //But any block later moved to a position fills it
-        moved_to.emplace(block.new_loc);
-    }
-
-    std::set<t_pl_loc> empty_locs;
-    std::set_difference(moved_from.begin(), moved_from.end(),
-                        moved_to.begin(), moved_to.end(),
-                        std::inserter(empty_locs, empty_locs.begin()));
-
-    return empty_locs;
-}
-
 #ifdef VTR_ENABLE_DEBUG_LOGGING
 void enable_placer_debug(const t_placer_opts& placer_opts,
                          ClusterBlockId blk_id) {

vpr/src/place/move_utils.h

@@ -125,8 +125,6 @@ e_block_move_result record_macro_self_swaps(t_pl_blocks_to_be_moved& blocks_affe
  */
 bool is_legal_swap_to_location(ClusterBlockId blk, t_pl_loc to);
 
-std::set<t_pl_loc> determine_locations_emptied_by_move(t_pl_blocks_to_be_moved& blocks_affected);
-
 /**
  * @brief Propose block for the RL agent based on required block type.
  *