[vpr][place] moved_block uses size() to indicate valid elements #2657

robluo · 2024-07-19T05:25:47Z

std::vector<t_pl_moved_block>& moved_blocks has as many entries as the total number of blocks, and we pass an integer to show how many entries are valid. size() can be used instead.

https://docs.google.com/spreadsheets/d/1CvJ-HcgW1FnkkrhhhtByHFcUm5zOhWdfVIwBDf5DpAI/edit?usp=sharing

The run time was not affected.

vaughnbetz · 2024-07-19T18:09:41Z

vpr/src/place/move_transactions.cpp

+}
+
+size_t t_pl_blocks_to_be_moved::get_size_and_increment() {
+    VTR_ASSERT(moved_blocks.size() < moved_blocks.capacity());


I'd make this a VTR_ASSERT_SAFE as it is in a hot routine (VTR_ASSERT_SAFE is taken out of release build).

vaughnbetz · 2024-07-19T18:15:45Z

vpr/src/place/move_transactions.cpp

@@ -40,7 +49,7 @@ 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) {
+    for (size_t iblk = 0; iblk < blocks_affected.moved_blocks.size(); ++iblk) {


To be safe, could do num_moved = blocks_affected.moved_blocks.size() before the loop -- it is slightly safer from a compiler optimization point of view, as otherwise we are counting on the compiler to figure out .size() doesn't need to be called each time.

vaughnbetz · 2024-07-19T18:16:18Z

vpr/src/place/move_transactions.cpp

@@ -67,7 +76,7 @@ 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 (int iblk = 0; iblk < blocks_affected.num_moved_blocks; ++iblk) {
+    for (size_t iblk = 0; iblk < blocks_affected.moved_blocks.size(); ++iblk) {


Same optimization (local variable)

vaughnbetz · 2024-07-19T18:16:54Z

vpr/src/place/move_transactions.cpp

@@ -97,7 +106,7 @@ 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 (int iblk = 0; iblk < blocks_affected.num_moved_blocks; ++iblk) {
+    for (size_t iblk = 0; iblk < blocks_affected.moved_blocks.size(); ++iblk) {


Same local loop count optimization.

Or use range-based for (Alex figured this out!)

vaughnbetz · 2024-07-19T18:20:25Z

vpr/src/place/move_utils.cpp

@@ -488,7 +488,7 @@ 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 (size_t iblk = 0; iblk < blocks_affected.moved_blocks.size(); ++iblk) {


range-based for

vaughnbetz · 2024-07-19T18:22:49Z

vpr/src/place/net_cost_handler.cpp

    /* Reset the net cost function flags first. */
-    for (int inet_affected = 0; inet_affected < num_nets_affected;
+    for (size_t inet_affected = 0; inet_affected < ts_nets_to_update.size();


range-based for

vaughnbetz · 2024-07-19T18:24:30Z

vpr/src/place/noc_place_utils.cpp

@@ -136,7 +136,7 @@ void find_affected_noc_routers_and_update_noc_costs(const t_pl_blocks_to_be_move
    affected_noc_links.clear();

    // go through the moved blocks and process them only if they are NoC routers
-    for (int iblk = 0; iblk < blocks_affected.num_moved_blocks; ++iblk) {
+    for (size_t iblk = 0; iblk < blocks_affected.moved_blocks.size(); ++iblk) {


range-based for

vaughnbetz · 2024-07-19T18:24:51Z

vpr/src/place/noc_place_utils.cpp

@@ -291,7 +291,7 @@ void revert_noc_traffic_flow_routes(const t_pl_blocks_to_be_moved& blocks_affect
    std::unordered_set<NocTrafficFlowId> reverted_traffic_flows;

    // go through the moved blocks and process them only if they are NoC routers
-    for (int iblk = 0; iblk < blocks_affected.num_moved_blocks; ++iblk) {
+    for (size_t iblk = 0; iblk < blocks_affected.moved_blocks.size(); ++iblk) {


range-based for

vaughnbetz · 2024-07-19T18:26:22Z

vpr/src/place/net_cost_handler.h

@@ -45,7 +45,7 @@ enum e_cost_methods {
 * @param timing_delta_c
 * @return The number of affected nets.
 */
-int find_affected_nets_and_update_costs(
+void find_affected_nets_and_update_costs(


Check if this function modifies (and doesn't clear/reset) the ts_* variables; if it changes their state (and leaves it changed) it should be mentioned briefly here.

vaughnbetz · 2024-07-19T18:26:49Z

vpr/src/place/place_constraints.h

@@ -100,7 +100,7 @@ void print_macro_constraint_error(const t_pl_macro& pl_macro);
 inline bool floorplan_legal(const t_pl_blocks_to_be_moved& blocks_affected) {
    bool floorplan_legal;

-    for (int i = 0; i < blocks_affected.num_moved_blocks; i++) {
+    for (size_t i = 0; i < blocks_affected.moved_blocks.size(); i++) {


range-based for

vaughnbetz · 2024-07-19T18:30:58Z

Looks good; please run on a single controlled machine (can be one design) to get a precise cpu time too, after the review comments.

Yulang Luo added 2 commits July 18, 2024 02:40

[vpr][place] ts_nets_to_update now uses size() instead of num_nets

54ba0fd

[vpr][place] moved_block uses size() to indicate valid elements

227187a

github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Jul 19, 2024

[vpr][place] remove unused include directory

dddbb29

Base automatically changed from placement_move_primitive to master July 19, 2024 18:03

vaughnbetz requested changes Jul 19, 2024

View reviewed changes

robluo closed this Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vpr][place] moved_block uses size() to indicate valid elements #2657

[vpr][place] moved_block uses size() to indicate valid elements #2657

robluo commented Jul 19, 2024 •

edited

Loading

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz Jul 19, 2024

vaughnbetz commented Jul 19, 2024

[vpr][place] moved_block uses size() to indicate valid elements #2657

[vpr][place] moved_block uses size() to indicate valid elements #2657

Conversation

robluo commented Jul 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

vaughnbetz commented Jul 19, 2024

robluo commented Jul 19, 2024 •

edited

Loading