Skip to content

Pack malloc to new #2084

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.

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 48 commits into from
Jul 28, 2022
Merged

Pack malloc to new #2084

merged 48 commits into from
Jul 28, 2022

Conversation

jmah76
Copy link
Contributor

@jmah76 jmah76 commented Jul 1, 2022

Description

Changed all the C style memory allocation in vpr/pack to C++ style memory allocation.

Checklist:

  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • X ] All new and existing tests passed

@github-actions github-actions bot added libarchfpga Library for handling FPGA Architecture descriptions libvtrutil VPR VPR FPGA Placement & Routing Tool labels Jul 1, 2022
@jmah76 jmah76 marked this pull request as ready for review July 4, 2022 15:43
@mithro mithro requested a review from hzeller July 6, 2022 00:41
@jmah76 jmah76 requested review from hzeller and removed request for hzeller July 6, 2022 14:19
Copy link
Contributor

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

Some initial pass. Mostly

  • Don't use parenthesis around the delete calls, as it might look like a function call, obscuring the language feature
  • various raw arrays (the ones that require delete[] might probably be worthwhile to be considered to be converted to containers (possibly in separate PRs)

@@ -298,7 +298,7 @@ static void free_all_pb_graph_nodes(std::vector<t_logical_block_type>& type_desc
if (type.pb_type) {
if (type.pb_graph_head) {
free_pb_graph(type.pb_graph_head);
vtr::free(type.pb_graph_head);
delete (type.pb_graph_head);
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter to all calls of delete is common to not have in parenthesis (otherwise it looks like a function call).
So I'd remove the parenthesis (throughout this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @hzeller. Jennifer has removed the brackets from delete (). For changing from raw arrays to containers: agree, but it will take a while so (as you suggest) it should be done in separate PRs and probably won't be all Jennifer.

if (pb_graph_node->input_pins[i][j].parent_pin_class)
vtr::free(pb_graph_node->input_pins[i][j].parent_pin_class);
delete[](pb_graph_node->input_pins[i][j].parent_pin_class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using delete[] indicates that this is some sort of array which might be an opportunity to replace with std::array or std::vector depending on the context. The requirement for delete[] should be rare if we have such containers (like what you did above already successfully with the pin_timing etc. containers)

I'd actually suggest to leave all remaining instances of vtr:;free() that would be replaced with delete[] out of this PR for now and deal with them separately and have dedicated PRs that either replace with containers or deliberately choose not to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be a good change but I'd like to get rid of our mixed use of free/delete/new/malloc (dangerous if the underlying new doesn't call malloc etc.) So I prefer to change them all now; Jennifer please file an issue to track changing specific variables to arrays / vectors.

@@ -236,31 +236,31 @@ void free_cluster_placement_stats(t_cluster_placement_stats* cluster_placement_s
cur = cluster_placement_stats_list[index].tried;
while (cur != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these lists look like hand-implemented linked lists.

It might be a consideration to implement them with std::list or std::forward_list.

(should be a consideration in a potential separate future PR). Here for now: only important to remove the parenthesis from the delete calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree: Jennifer please file a follow-up issue to track this.

@@ -189,8 +189,9 @@ std::map<t_logical_block_type_ptr, size_t> do_clustering(const t_packer_opts& pa
get_max_cluster_size_and_pb_depth(helper_ctx.max_cluster_size, max_pb_depth);

if (packer_opts.hill_climbing_flag) {
clustering_data.hill_climbing_inputs_avail = (int*)vtr::calloc(helper_ctx.max_cluster_size + 1,
sizeof(int));
clustering_data.hill_climbing_inputs_avail = new int[helper_ctx.max_cluster_size + 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

could the hill_climbing_inputs_avail be a std::vector ? They are automatically 0 initialized and we just would call resize(helper_ctx.max_cluster_size + 1) here. (while at it, it might be worthwhile commenting why + 1)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be doable; another issue to file Jennifer since I think we should land this PR first.

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'll be honest, I don't know why + 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't recall either; indexed up to max_cluster_size presumably but I'd have to look at the code to see why.

@vaughnbetz
Copy link
Contributor

CI failures seem to be due to machines not available; try rerunning failed tests once the others complete.

@jmah76
Copy link
Contributor Author

jmah76 commented Jul 7, 2022

Will file issues on the requested changes.

@jmah76
Copy link
Contributor Author

jmah76 commented Jul 8, 2022

@vaughnbetz Reran the failed tests and they've all passed. I think this is ready to merge.

@vaughnbetz
Copy link
Contributor

Same (paranoid) thing to test: do a QoR run and attach results. OK if that run includes both sets of changes.

@jmah76
Copy link
Contributor Author

jmah76 commented Jul 25, 2022

Full QoR results are on my wiki page.
image

@vaughnbetz vaughnbetz merged commit 7989197 into master Jul 28, 2022
@vaughnbetz vaughnbetz deleted the pack_malloc_to_new branch July 28, 2022 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libarchfpga Library for handling FPGA Architecture descriptions libvtrutil VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants