apply PR comments

Author: soheilshahrouz

libs/libvtrutil/src/vtr_memory.h

@@ -14,7 +14,7 @@ namespace vtr {
 /**
  * @brief This function will force the container to be cleared
  *
- * It release its held memory.
+ * It releases its held memory.
  * For efficiency, STL containers usually don't
  * release their actual heap-allocated memory until
  * destruction (even if Container::clear() is called).

vpr/src/base/blk_loc_registry.cpp

@@ -4,6 +4,9 @@
 #include "move_transactions.h"
 #include "globals.h"
 
+BlkLocRegistry::BlkLocRegistry()
+    : expected_transaction_(e_expected_transaction::APPLY) {}
+
 const vtr::vector_map<ClusterBlockId, t_block_loc>& BlkLocRegistry::block_locs() const {
     return  block_locs_;
 }
@@ -118,6 +121,8 @@ void BlkLocRegistry::place_sync_external_block_connections(ClusterBlockId iblk)
 void BlkLocRegistry::apply_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {
     auto& device_ctx = g_vpr_ctx.device();
 
+    VTR_ASSERT_DEBUG(expected_transaction_ == e_expected_transaction::APPLY);
+
     // 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 t_pl_moved_block& moved_block : blocks_affected.moved_blocks) {
@@ -139,9 +144,13 @@ void BlkLocRegistry::apply_move_blocks(const t_pl_blocks_to_be_moved& blocks_aff
             place_sync_external_block_connections(blk);
         }
     }
+
+    expected_transaction_ = e_expected_transaction::COMMIT_REVERT;
 }
 
 void BlkLocRegistry::commit_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {
+    VTR_ASSERT_DEBUG(expected_transaction_ == e_expected_transaction::COMMIT_REVERT);
+
     // Swap physical location
     for (const t_pl_moved_block& moved_block : blocks_affected.moved_blocks) {
         ClusterBlockId blk = moved_block.block_num;
@@ -163,11 +172,15 @@ void BlkLocRegistry::commit_move_blocks(const t_pl_blocks_to_be_moved& blocks_af
         grid_blocks_.set_block_at_location(to, blk);
 
     } // Finish updating clb for all blocks
+
+    expected_transaction_ = e_expected_transaction::APPLY;
 }
 
 void BlkLocRegistry::revert_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {
     auto& device_ctx = g_vpr_ctx.device();
 
+    VTR_ASSERT_DEBUG(expected_transaction_ == e_expected_transaction::COMMIT_REVERT);
+
     // Swap the blocks back, nets not yet swapped they don't need to be changed
     for (const t_pl_moved_block& moved_block : blocks_affected.moved_blocks) {
         ClusterBlockId blk = moved_block.block_num;
@@ -191,4 +204,6 @@ void BlkLocRegistry::revert_move_blocks(const t_pl_blocks_to_be_moved& blocks_af
         VTR_ASSERT_SAFE_MSG(grid_blocks_.block_at_location(old_loc) == blk,
                             "Grid blocks should only have been updated if swap committed (not reverted)");
     }
+
+    expected_transaction_ = e_expected_transaction::APPLY;
 }

vpr/src/base/blk_loc_registry.h

@@ -20,7 +20,7 @@ struct t_pl_blocks_to_be_moved;
  */
 class BlkLocRegistry {
   public:
-    BlkLocRegistry() = default;
+    BlkLocRegistry();
     ~BlkLocRegistry() = default;
     BlkLocRegistry(const BlkLocRegistry&) = delete;
     BlkLocRegistry& operator=(const BlkLocRegistry&) = default;
@@ -83,22 +83,38 @@ class BlkLocRegistry {
     void place_sync_external_block_connections(ClusterBlockId iblk);
 
     /**
-     * @brief Moves the blocks in blocks_affected to their new locations
+     * @brief Moves the blocks in blocks_affected to their new locations.
+     * This method only updates `grid_blocks_` member variable and not `grid_blocks_`.
+     * After this method is called, either commit_move_blocks() or revert_move_blocks()
+     * must be called to either revert the new locations or commit them to `grid_block_`
      * @param blocks_affected Clustered blocks affected by a swap and their old and new locations.
      */
     void apply_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected);
 
     /**
      * @brief Commits the blocks in blocks_affected to their new locations (updates inverse lookups in grid_blocks)
+     * This method can only be called after a call to apply_move_blocks() to commit the block location changes
+     * to `grid_block_`. If this method is called after apply_move_blocks(), revert_move_blocks() must not be called
+     * until the next call to apply_move_blocks() is made.
      * @param blocks_affected Clustered blocks affected by a swap and their old and new locations.
      */
     void commit_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected);
 
     /**
-     * @brief Moves the blocks in blocks_affected to their old locations
+     * @brief Moves the blocks in blocks_affected to their old locations by updating `block_locs_` member variable.
+     * This method can only be called after a call to apply_move_blocks() to revert the block location changes
+     * applied to `block_locs_`. If this method is called after apply_move_blocks(), commit_move_blocks() must not be called
+     * until the next call to apply_move_blocks() is made.
      * @param blocks_affected Clustered blocks affected by a swap and their old and new locations.
      */
     void revert_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected);
+
+    enum class e_expected_transaction {
+        APPLY,
+        COMMIT_REVERT
+    };
+
+    e_expected_transaction expected_transaction_;
 };
 
 #endif //VTR_BLK_LOC_REGISTRY_H

vpr/src/place/move_generator.cpp

@@ -6,6 +6,16 @@
 void MoveGenerator::calculate_reward_and_process_outcome(const MoveOutcomeStats& move_outcome_stats,
                                                          double delta_c,
                                                          float timing_bb_factor) {
+    /*
+     * To learn about different reward functions refer to the following paper:
+     * Elgammal MA, Murray KE, Betz V. RLPlace: Using reinforcement learning and
+     * smart perturbations to optimize FPGA placement.
+     * IEEE Transactions on Computer-Aided Design of Integrated Circuits and Systems.
+     * 2021 Sep 3;41(8):2532-45.
+     *
+     * For runtime-aware reward function, the reward value is divided by a normalized
+     * runtime in the implementation of process_outcome()
+     */
     switch (reward_func_) {
         case e_reward_function::WL_BIASED_RUNTIME_AWARE:
             if (delta_c < 0) {
@@ -25,6 +35,7 @@ void MoveGenerator::calculate_reward_and_process_outcome(const MoveOutcomeStats&
             process_outcome(-1 * delta_c, reward_func_);
             break;
 
+
         case e_reward_function::NON_PENALIZING_BASIC:
         case e_reward_function::RUNTIME_AWARE:
             if (delta_c < 0) {

vpr/src/place/net_cost_handler.h

@@ -1,3 +1,9 @@
+/**
+ * @file net_cost_handler.h
+ * @brief This file contains the declaration of NetCostHandler class used to update placement cost when a new move is proposed/committed.
+ * For more details on the overall algorithm, refer to the comment at the top of the net_cost_handler.cpp
+ */
+
 #pragma once
 
 #include "place_delay_model.h"
@@ -38,9 +44,10 @@ class NetCostHandler {
     NetCostHandler& operator=(NetCostHandler&&) = delete;
 
     /**
-     * @brief Resize temporary swap data structures needed to determine which nets are affected by a move and data needed per net
-     * about where their terminals are in order to quickly (incrementally) update their wirelength costs. These data structures are
-     * (layer_)ts_bb_edge_new, (layer_)ts_bb_coord_new, ts_layer_sink_pin_count, and ts_nets_to_update.
+     * @brief Initializes a NetCostHandler object, which contains temporary swap data structures needed to determine which nets
+     * are affected by a move and data needed per net about where their terminals are in order to quickly (incrementally) update
+     * their wirelength costs. These data structures are (layer_)ts_bb_edge_new, (layer_)ts_bb_coord_new, ts_layer_sink_pin_count,
+     * and ts_nets_to_update.
      * @param num_nets Number of nets in the netlist used by the placement engine (currently clustered netlist)
      * @param cube_bb True if the 3D bounding box should be used, false otherwise.
      * @param place_cost_exp It is an exponent to which you take the average inverse channel
@@ -423,7 +430,20 @@ class NetCostHandler {
                                              std::vector<t_2D_bb>& bb_edge_new,
                                              std::vector<t_2D_bb>& bb_coord_new);
 
+     /**
+      * @brief Computes the bounding box from scratch using 2D bounding boxes (per-layer mode)
+      * @param method The method used to calculate placement cost. Specifies whether the cost is
+      * computed from scratch or incrementally.
+      * @return Computed bounding box cost.
+      */
      double comp_per_layer_bb_cost_(e_cost_methods method);
+
+     /**
+      * @brief Computes the bounding box from scratch using 3D bounding boxes (cube mode)
+      * @param method The method used to calculate placement cost. Specifies whether the cost is
+      * computed from scratch or incrementally.
+      * @return Computed bounding box cost.
+      */
      double comp_cube_bb_cost_(e_cost_methods method);
 
      /**