Pack malloc to new #2084

jmah76 · 2022-07-01T13:35:14Z

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

…uting/vtr-verilog-to-routing into pack_malloc_to_new

hzeller

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)

hzeller · 2022-07-06T16:41:42Z

libs/libarchfpga/src/arch_util.cpp

@@ -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);


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).

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.

hzeller · 2022-07-06T16:46:46Z

libs/libarchfpga/src/arch_util.cpp

            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);


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.

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.

hzeller · 2022-07-06T16:51:35Z

vpr/src/base/vpr_types.cpp

@@ -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) {


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.

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

hzeller · 2022-07-06T16:55:17Z

vpr/src/pack/cluster.cpp

@@ -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];


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)

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

I'll be honest, I don't know why + 1.

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

vaughnbetz · 2022-07-07T19:00:10Z

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

jmah76 · 2022-07-07T19:00:34Z

Will file issues on the requested changes.

jmah76 · 2022-07-08T18:10:23Z

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

vaughnbetz · 2022-07-21T19:15:56Z

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

jmah76 · 2022-07-25T13:53:02Z

Full QoR results are on my wiki page.

jmah76 added 21 commits June 24, 2022 14:21

initial commit

ef74b79

cluster_feasibility_filter.cpp

8e99ecd

cluster_placements

8108924

cluster_utils

ca9a728

cluster & lb_type_rr_graph

ef28b6e

output_clustering & pb_type_graph_annotations

45f4a29

pb_type_graph vectors

d80a8b3

pb_type_graph some mallocs & callocs

2d11144

more pb_type_graph mallocs & callocs

1b6daed

more pb_type_graph

f0becd3

pb_type_graph clock, input, output pins

03af3f5

pb_type_graph most mem

40d6557

prepack some

6da52ed

prepack.cpp

caf0b82

deleted unwantedfiles

bbb5356

formatting

9d0d3c2

fixed QoR fails(?)

80d639d

fixed bad array new length errors

10135ae

formatting fix

fc15b62

reallocs to vectors

e202d6e

formatting

9031bd4

github-actions bot added libarchfpga Library for handling FPGA Architecture descriptions libvtrutil VPR VPR FPGA Placement & Routing Tool labels Jul 1, 2022

jmah76 added 6 commits July 1, 2022 12:28

formatting

1cb521d

initial commit

047c641

cluster_feasibility_filter.cpp

16b891b

cluster_placements

697d0b1

cluster_utils

a2df349

cluster & lb_type_rr_graph

439e202

jmah76 added 13 commits July 1, 2022 14:24

pb_type_graph clock, input, output pins

d05df70

pb_type_graph most mem

6c37a2c

prepack some

2b35adb

prepack.cpp

f4ba119

deleted unwantedfiles

05ac251

formatting

057b77b

fixed QoR fails(?)

aa4bbbb

fixed bad array new length errors

8cda78a

formatting fix

466c717

reallocs to vectors

e691124

formatting

7cbddc7

formatting

c31469a

Merge branch 'pack_malloc_to_new' of https://github.com/verilog-to-ro…

2ab89a1

…uting/vtr-verilog-to-routing into pack_malloc_to_new

jmah76 marked this pull request as ready for review July 4, 2022 15:43

mithro requested a review from hzeller July 6, 2022 00:41

jmah76 requested review from hzeller and removed request for hzeller July 6, 2022 14:19

hzeller reviewed Jul 6, 2022

View reviewed changes

removed brackets around objs to be deleted

5366e76

formatting

4be872f

This was referenced Jul 7, 2022

VPR Pack: C-style Arrays to C++ Vectors #2087

Open

VPR Pack: Replace hand-implemented linked lists to std::lists #2088

Open

Merge branch 'master' into pack_malloc_to_new

5f46eb7

vaughnbetz merged commit 7989197 into master Jul 28, 2022

vaughnbetz deleted the pack_malloc_to_new branch July 28, 2022 19:19

		@@ -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) {

Pack malloc to new #2084

Pack malloc to new #2084

Uh oh!

Conversation

jmah76 commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

hzeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Jul 7, 2022

Uh oh!

jmah76 commented Jul 7, 2022

Uh oh!

jmah76 commented Jul 8, 2022

Uh oh!

vaughnbetz commented Jul 21, 2022

Uh oh!

jmah76 commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jmah76 commented Jul 1, 2022 •

edited

Loading

jmah76 commented Jul 25, 2022 •

edited

Loading