[vpr][place] Placement move primitive refactoring #2659

robluo · 2024-07-22T19:05:23Z

In "net_cost_handler.cpp", there are three related global variables: net_cost, proposed_net_cost, and bb_updated_before. We can organize these by putting them into a struct called t_net_cost_data. You may also rename bb_updated_before to bb_update_status.
In placement, we use two methods to update the bounding box: the 3D bounding box (also used for 2D architecture) and the per-layer bounding box. When a swap happens, multiple variables are updated, indicated by the ts (temporary swap) prefix. We can put all these variables into a class. Since only a subset is used depending on the bounding box type, you can examine how these variables are updated throughout the file and write methods for this class to update the appropriate ones.
For runtime reasons, we initialize ts_nets_to_update to the number of nets in the netlist and use a variable num_affected_nets to indicate how many entries are valid. You can experiment to see how runtime is affected if we remove this variable and just reserve elements in this vector and use .size().
The same approach is used for std::vector<t_pl_moved_block>& moved_blocks, which has as many entries as the total number of blocks, and we pass an integer to show how many entries are valid. You can also run an experiment to see how much runtime is affected if you remove this.

QoR bitcoin_miner_stratixiv_arch_timing (final wire length remains the same):
baseline: packing: 354.64, placement: 3706.12, routing 409.20, vpr flow: 4930.11
modified: packing: 356.83, placement: 3777.65, routing 412.23, vpr flow: 4994.61

soheilshahrouz

I added some comments for all files other than net_cost_handler.cpp/.h

soheilshahrouz · 2024-07-27T18:10:33Z

vpr/src/place/move_transactions.h

    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();


please add doxygen documentation for this method.

soheilshahrouz · 2024-07-27T18:11:28Z

vpr/src/place/move_transactions.h

 * moved blocks: a list of moved blocks data structure with     *
 *               information on the move.                       *
 *               [0...max_blocks-1]                       *
 * affected_pins: pins affected by this move (used to           *
 *                incrementally invalidate parts of the timing  *
 *                graph.                                        */
 struct t_pl_blocks_to_be_moved {
-    explicit t_pl_blocks_to_be_moved(size_t max_blocks)
-        : moved_blocks(max_blocks) {}
+    explicit t_pl_blocks_to_be_moved(size_t max_blocks);


it is probably a good idea to delete the default constructor if the objects of this type are always expected to be initialzed with a non-zero capacity

Also delete the copy constructor to avoid unintended copies.

soheilshahrouz · 2024-07-27T18:14:31Z

vpr/src/place/move_transactions.cpp

+
+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);


is calling resize() necessary?

No, it is not necessary, the vector default constructor will set size to 0.

No, it is not necessary, the vector default constructor will set size to 0.

So I think it is better to remove it

vpr/src/place/move_transactions.cpp

soheilshahrouz · 2024-07-27T18:22:08Z

vpr/src/place/move_transactions.cpp

@@ -40,11 +51,11 @@ void apply_move_blocks(const t_pl_blocks_to_be_moved& blocks_affected) {

    //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 (int iblk = 0; iblk < blocks_affected.num_moved_blocks; ++iblk) {
-        ClusterBlockId blk = blocks_affected.moved_blocks[iblk].block_num;
+    for (const auto& block : blocks_affected.moved_blocks) {


I think if the element type (t_pl_moved_block) is explicitly specified instead of using auto keyword, the code would be more readable, especially that the type name is not that long.

I also suggest that the element name changes from "block" to "moved_block"

soheilshahrouz · 2024-07-27T18:40:45Z

vpr/src/place/move_utils.cpp

@@ -488,12 +488,12 @@ std::set<t_pl_loc> determine_locations_emptied_by_move(t_pl_blocks_to_be_moved&
    std::set<t_pl_loc> moved_from;
    std::set<t_pl_loc> moved_to;

-    for (int iblk = 0; iblk < blocks_affected.num_moved_blocks; ++iblk) {
+    for (const auto& block : blocks_affected.moved_blocks) {


using the type name explicitly makes the code more readable

soheilshahrouz · 2024-07-27T18:48:16Z

vpr/src/place/move_utils.cpp

@@ -488,12 +488,12 @@ std::set<t_pl_loc> determine_locations_emptied_by_move(t_pl_blocks_to_be_moved&
    std::set<t_pl_loc> moved_from;


blocks_affected can be passed by const reference

this function can be implemented as member function in t_pl_blocks_to_be_moved

soheilshahrouz · 2024-07-27T18:50:33Z

vpr/src/place/place_constraints.h

-    for (int i = 0; i < blocks_affected.num_moved_blocks; i++) {
-        floorplan_legal = cluster_floorplanning_legal(blocks_affected.moved_blocks[i].block_num,
-                                                      blocks_affected.moved_blocks[i].new_loc);
+    for (const auto& block : blocks_affected.moved_blocks) {


explicit type name make the code more readable

soheilshahrouz · 2024-07-27T18:54:43Z

vpr/src/place/place.cpp

@@ -1378,7 +1378,7 @@ static e_move_result try_swap(const t_annealing_state* state,
        //
        //Also find all the pins affected by the swap, and calculates new connection
        //delays and timing costs and store them in proposed_* data structures.
-        int num_nets_affected = find_affected_nets_and_update_costs(
+        find_affected_nets_and_update_costs(


I think if this function call is written in two lines, it would be more readable:

find_affected_nets_and_update_costs(place_algorithm, delay_model, criticalities, blocks_affected, bb_delta_c, timing_delta_c);

soheilshahrouz · 2024-07-27T18:55:46Z

vpr/src/place/place.cpp

@@ -1569,7 +1568,7 @@ static e_move_result try_swap(const t_annealing_state* state,
        calculate_reward_and_process_outcome(placer_opts, move_outcome_stats,
                                             delta_c, timing_bb_factor, move_generator);
    } else {
-//        std::cout << "Group move delta cost: " << delta_c << std::endl;
+        //        std::cout << "Group move delta cost: " << delta_c << std::endl;


remove the else statement
I used it for debugging, but it somehow found its way into master

inet_cost, proposed_net_cost, and bb_updated_before grouped in a struct. bb_updated_before renamed to bb_update_status.

soheilshahrouz · 2024-07-27T20:30:00Z