-
Notifications
You must be signed in to change notification settings - Fork 415
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
Pack malloc to new #2084
Conversation
There was a problem hiding this 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)
libs/libarchfpga/src/arch_util.cpp
Outdated
@@ -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); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
libs/libarchfpga/src/arch_util.cpp
Outdated
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
CI failures seem to be due to machines not available; try rerunning failed tests once the others complete. |
Will file issues on the requested changes. |
@vaughnbetz Reran the failed tests and they've all passed. I think this is ready to merge. |
Same (paranoid) thing to test: do a QoR run and attach results. OK if that run includes both sets of changes. |
Description
Changed all the C style memory allocation in vpr/pack to C++ style memory allocation.
Checklist: