From 6be051ec9676c7fd382b1176e02e7930e6461037 Mon Sep 17 00:00:00 2001 From: Fred Tombs Date: Wed, 16 Apr 2025 16:36:53 -0400 Subject: [PATCH 1/3] Inverse use of macro_can_be_placed argument check_all_legality to align with meaning --- vpr/src/place/analytic_placer.cpp | 4 ++-- vpr/src/place/initial_placement.cpp | 9 ++++----- vpr/src/place/place_util.cpp | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/vpr/src/place/analytic_placer.cpp b/vpr/src/place/analytic_placer.cpp index a0897af6785..e460c5bd58f 100644 --- a/vpr/src/place/analytic_placer.cpp +++ b/vpr/src/place/analytic_placer.cpp @@ -411,7 +411,7 @@ void AnalyticPlacer::setup_solve_blks(t_logical_block_type_ptr blkTypes) { void AnalyticPlacer::update_macros() { for (auto& macro : place_macros_.macros()) { ClusterBlockId head_id = macro.members[0].blk_index; - bool mac_can_be_placed = macro_can_be_placed(macro, blk_locs[head_id].loc, true, blk_loc_registry_ref_); + bool mac_can_be_placed = macro_can_be_placed(macro, blk_locs[head_id].loc, false, blk_loc_registry_ref_); //if macro can not be placed in this head pos, change the head pos if (!mac_can_be_placed) { @@ -420,7 +420,7 @@ void AnalyticPlacer::update_macros() { } //macro should be placed successfully after changing the head position - VTR_ASSERT(macro_can_be_placed(macro, blk_locs[head_id].loc, true, blk_loc_registry_ref_)); + VTR_ASSERT(macro_can_be_placed(macro, blk_locs[head_id].loc, false, blk_loc_registry_ref_)); //update other member's location based on head pos for (auto member = ++macro.members.begin(); member != macro.members.end(); ++member) { diff --git a/vpr/src/place/initial_placement.cpp b/vpr/src/place/initial_placement.cpp index aac91e0fd65..5fb0d5f1734 100644 --- a/vpr/src/place/initial_placement.cpp +++ b/vpr/src/place/initial_placement.cpp @@ -742,12 +742,10 @@ static inline t_pl_loc find_nearest_compatible_loc(const t_flat_pl_loc& src_flat // floorplanning constraints and compatibility for all // members of the macro. This prevents some macros being // placed where they obviously cannot be implemented. - // Note: The check_all_legality flag is poorly named. false means - // that it WILL check all legality... t_pl_loc new_loc = t_pl_loc(grid_loc.x, grid_loc.y, new_sub_tile, grid_loc.layer_num); bool site_legal_for_macro = macro_can_be_placed(pl_macro, new_loc, - false /*check_all_legality*/, + true /*check_all_legality*/, blk_loc_registry); if (site_legal_for_macro) { // Update the best solition. @@ -1210,9 +1208,10 @@ bool try_place_macro(const t_pl_macro& pl_macro, return macro_placed; } - bool mac_can_be_placed = macro_can_be_placed(pl_macro, head_pos, /*check_all_legality=*/false, blk_loc_registry); + // called from initial placement + bool macro_can_be_placed = macro_can_be_placed(pl_macro, head_pos, /*check_all_legality=*/true, blk_loc_registry); - if (mac_can_be_placed) { + if (macro_can_be_placed) { // Place down the macro macro_placed = true; VTR_LOGV_DEBUG(f_placer_debug, "\t\t\t\tMacro is placed at the given location\n"); diff --git a/vpr/src/place/place_util.cpp b/vpr/src/place/place_util.cpp index 1ac0899fbdf..ad201120acb 100644 --- a/vpr/src/place/place_util.cpp +++ b/vpr/src/place/place_util.cpp @@ -191,7 +191,7 @@ bool macro_can_be_placed(const t_pl_macro& pl_macro, * floorplan constraint is not supported by analytical placement yet, * hence, if macro_can_be_placed is called from analytical placer, no further actions are required. */ - if (check_all_legality) { + if (not check_all_legality) { continue; } From 08c18099282ab339b23aa4130147f2db9e2281b4 Mon Sep 17 00:00:00 2001 From: Fred Tombs Date: Wed, 23 Apr 2025 15:24:34 -0400 Subject: [PATCH 2/3] Invalid C++ fix --- vpr/src/place/initial_placement.cpp | 6 ++---- vpr/src/place/place_util.h | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/vpr/src/place/initial_placement.cpp b/vpr/src/place/initial_placement.cpp index 5fb0d5f1734..7f658ccb968 100644 --- a/vpr/src/place/initial_placement.cpp +++ b/vpr/src/place/initial_placement.cpp @@ -1209,10 +1209,8 @@ bool try_place_macro(const t_pl_macro& pl_macro, } // called from initial placement - bool macro_can_be_placed = macro_can_be_placed(pl_macro, head_pos, /*check_all_legality=*/true, blk_loc_registry); - - if (macro_can_be_placed) { - // Place down the macro + if (macro_can_be_placed(pl_macro, head_pos, /*check_all_legality=*/true, blk_loc_registry)) { + // Place down the macromacro_can_be_placed macro_placed = true; VTR_LOGV_DEBUG(f_placer_debug, "\t\t\t\tMacro is placed at the given location\n"); for (const t_pl_macro_member& pl_macro_member : pl_macro.members) { diff --git a/vpr/src/place/place_util.h b/vpr/src/place/place_util.h index 14cf44455c6..6faa963106a 100644 --- a/vpr/src/place/place_util.h +++ b/vpr/src/place/place_util.h @@ -261,7 +261,7 @@ inline bool is_loc_on_chip(t_physical_tile_loc loc) { * determines whether the routine should check all legality constraint * Analytic placer does not require to check block's capacity or * floorplanning constraints. However, initial placement or SA-based approach - * require to check for all legality constraints. + * require checking all legality constraints. * @param blk_loc_registry Placement block location information. * */ From ec8d7d87424a1b934155b7d4e9fbe2cabe891e86 Mon Sep 17 00:00:00 2001 From: Fred Tombs Date: Wed, 30 Apr 2025 11:50:57 -0400 Subject: [PATCH 3/3] fix comments from alex --- vpr/src/place/initial_placement.cpp | 2 -- vpr/src/place/place_util.cpp | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/vpr/src/place/initial_placement.cpp b/vpr/src/place/initial_placement.cpp index 7f658ccb968..312ba533612 100644 --- a/vpr/src/place/initial_placement.cpp +++ b/vpr/src/place/initial_placement.cpp @@ -1208,9 +1208,7 @@ bool try_place_macro(const t_pl_macro& pl_macro, return macro_placed; } - // called from initial placement if (macro_can_be_placed(pl_macro, head_pos, /*check_all_legality=*/true, blk_loc_registry)) { - // Place down the macromacro_can_be_placed macro_placed = true; VTR_LOGV_DEBUG(f_placer_debug, "\t\t\t\tMacro is placed at the given location\n"); for (const t_pl_macro_member& pl_macro_member : pl_macro.members) { diff --git a/vpr/src/place/place_util.cpp b/vpr/src/place/place_util.cpp index ad201120acb..7fdf3383a06 100644 --- a/vpr/src/place/place_util.cpp +++ b/vpr/src/place/place_util.cpp @@ -191,7 +191,7 @@ bool macro_can_be_placed(const t_pl_macro& pl_macro, * floorplan constraint is not supported by analytical placement yet, * hence, if macro_can_be_placed is called from analytical placer, no further actions are required. */ - if (not check_all_legality) { + if (!check_all_legality) { continue; }