Skip to content

[Place] rename get_initial_move_lim to get_place_inner_loop_num_move #2955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 8, 2025
Merged

[Place] rename get_initial_move_lim to get_place_inner_loop_num_move #2955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7697257
[vpr][place] rename get_initial_move_lim to get_place_inner_loop_num_…
amin1377 Mar 31, 2025
37c5fe1
fix a typo
amin1377 Mar 31, 2025
3f8d8e4
Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…
amin1377 Apr 4, 2025
e6c296e
[vpr][place] update get_place_inner_loop_num_move comment
amin1377 Apr 4, 2025
7e9e587
[vpr][place] prrint number of moves per temp after getting the number
amin1377 Apr 6, 2025
292cd56
Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…
amin1377 Apr 6, 2025
b154328
make format
amin1377 Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion vpr/src/place/annealer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,9 @@ PlacementAnnealer::PlacementAnnealer(const t_placer_opts& placer_opts,
first_crit_exponent = 0.f;
}

int first_move_lim = get_initial_move_lim(placer_opts, placer_opts_.anneal_sched);
int first_move_lim = get_place_inner_loop_num_move(placer_opts, placer_opts_.anneal_sched);

VTR_LOG("Moves per temperature: %d\n", first_move_lim);

if (placer_opts.inner_loop_recompute_divider != 0) {
inner_recompute_limit_ = static_cast<int>(0.5 + (float)first_move_lim / (float)placer_opts.inner_loop_recompute_divider);
Expand Down
4 changes: 1 addition & 3 deletions vpr/src/place/place_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ t_placer_costs& t_placer_costs::operator+=(const NocCostTerms& noc_delta_cost) {
return *this;
}

int get_initial_move_lim(const t_placer_opts& placer_opts, const t_annealing_sched& annealing_sched) {
int get_place_inner_loop_num_move(const t_placer_opts& placer_opts, const t_annealing_sched& annealing_sched) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably renaming it to comp_place_inner_loop_num_move() makes it clear that it actually computes the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't see that much difference between these two names :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can go either way. For me, "get_..." methods are usually O(1) lookups. Whereas "calc_..." is used to calculate some value and is usually more expensive. It implicitly says that this function is not as cheap to call and care should be taken when putting it in a loop.

I used the same practice here: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/85dfc2983cc74150959aca1ff7666c9b7c292fd7/vpr/src/analytical_place/flat_placement_density_manager.cpp

Although this function is cheap and is O(1), there is some complexity in here that implies that it shouldn't be called many times (For example the log message). Up to you honestly how you want to name this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a good way to decide whether to name a function calc_* or get_*. Thanks, Alex!

I think we shouldn’t print the log message inside the helper function. I’ve removed it from there and instead added the log in the annealer constructor where the function is called.

const auto& device_ctx = g_vpr_ctx.device();
const auto& cluster_ctx = g_vpr_ctx.clustering();

Expand All @@ -68,8 +68,6 @@ int get_initial_move_lim(const t_placer_opts& placer_opts, const t_annealing_sch
/* Avoid having a non-positive move_lim */
move_lim = std::max(move_lim, 1);

VTR_LOG("Moves per temperature: %d\n", move_lim);

return move_lim;
}

Expand Down
5 changes: 3 additions & 2 deletions vpr/src/place/place_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,8 @@ class t_placer_statistics {
};

/**
* @brief Get the initial limit for inner loop block move attempt limit.
* @brief Get the number of moves attempted by the annealer's inner
* loop in each outer loop iteration.
*
* There are two ways to scale the move limit.
* e_place_effort_scaling::CIRCUIT
Expand All @@ -210,7 +211,7 @@ class t_placer_statistics {
* (device_size >> num_blocks), the search space is larger, so the second method
* performs more moves to ensure better optimization.
*/
int get_initial_move_lim(const t_placer_opts& placer_opts, const t_annealing_sched& annealing_sched);
int get_place_inner_loop_num_move(const t_placer_opts& placer_opts, const t_annealing_sched& annealing_sched);

/**
* @brief Returns the standard deviation of data set x.
Expand Down