[WIP] Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc #3040

SamuelHo10 · 2025-05-14T15:29:55Z

Replaced most of the free/malloc/calloc with new/delete in ArchFPGA library. I left a few unchanged because they either conflict with vtr::strdup or are used by other libraries such as VPR or Odin. This is my first pull request, so please let me know if I am forgetting anything.

…g-to-routing/vtr-verilog-to-routing into eliminate_free_and_malloc

soheilshahrouz

Thanks @SamuelHo10

I left a few comments suggesting how the code can be improved in future PRs.

soheilshahrouz · 2025-05-16T17:18:29Z

libs/libarchfpga/src/arch_util.cpp

@@ -163,7 +163,7 @@ void free_arch(t_arch* arch) {
    vtr::free(arch->architecture_id);

    if (arch->clocks) {
-        vtr::free(arch->clocks->clock_inf);
+        delete[] arch->clocks->clock_inf;


t_clock_arch holds an array of t_clock_network elements. It can be replaced with std::vector<t_clock_network>.

I think we should get rid of t_clock_arch struct

soheilshahrouz · 2025-05-16T17:26:53Z

libs/libarchfpga/src/arch_util.cpp

-                vtr::free(pb_type->modes[i].interconnect[j].annotations[k].prop);
-                vtr::free(pb_type->modes[i].interconnect[j].annotations[k].value);
+                delete[] pb_type->modes[i].interconnect[j].annotations[k].prop;
+                delete[] pb_type->modes[i].interconnect[j].annotations[k].value;


Instead of using int* and char** in t_pin_to_pin_annotation, we can use a std::vector<std::pair<e_pin_to_pin_delay_annotations, std::string>>. This way, when a t_pin_to_pin_annotation object goes out of scope, prop-value pairs are deleted automatically.

soheilshahrouz · 2025-05-16T17:31:39Z

libs/libarchfpga/src/echo_arch.cpp

@@ -383,7 +383,7 @@ static void print_model(FILE* echo, const t_model& model) {
 static void PrintPb_types_rec(FILE* Echo, const t_pb_type* pb_type, int level, const LogicalModels& models) {
    char* tabs;

-    tabs = (char*)vtr::malloc((level + 1) * sizeof(char));
+    tabs = new char[level + 1];


We should replace this with std::string to make the lifetime management automatic.
You can use c_str() method to pass the underlying char* pointer to fprintf()

vaughnbetz

Thanks for starting the cleanup on this @SamuelHo10 .
I think some of these changes are unsafe in isolation though ... see my comments on calloc and default constructors. I think we need to write default constructors for the structs in question to be able to safely use new instead of calloc.
We should also take this as an opportunity to get rid of some complexity in the code and simplify memory management by moving to std:: container classes like vectors as @soheilshahrouz suggested. I think you should take this changes a little at a time though, running tests (or pushing to CI to have it run tests) frequently, and committing intermediate cleanups at each working stage.

We could discuss in person next week what good stages are.

vaughnbetz · 2025-05-16T21:23:05Z

libs/libarchfpga/src/arch_util.cpp

@@ -495,13 +495,12 @@ void alloc_and_load_default_child_for_pb_type(t_pb_type* pb_type,
    copy->num_pb = 1;

    /* Power */
-    copy->pb_type_power = (t_pb_type_power*)vtr::calloc(1,
-                                                        sizeof(t_pb_type_power));
+    copy->pb_type_power = new t_pb_type_power();


calloc 0-initializes the data it allocates. Looking at the code in physical_types.h, is no default constructor (i.e. the constructor with no arguments) for t_pb_type_power defined. Hence the compiler will make one. That constructor does not initialize built in types like integers and floats.

Hence this code change is dangerous: we have gone from a block of 0-initialized memory to a struct that has unknown state. Probably @jgoeders used calloc on purpose to initialize the power elements to 0.

To convert calloc uses like this to new, you have to make a default constructor for the struct/class in question and initialize all its data members to 0.

vaughnbetz · 2025-05-16T21:23:53Z

libs/libarchfpga/src/arch_util.cpp

    copy->pb_type_power->estimation_method = power_method_inherited(pb_type->pb_type_power->estimation_method);

    /* Ports */
    copy->num_ports = pb_type->num_ports;
-    copy->ports = (t_port*)vtr::calloc(pb_type->num_ports, sizeof(t_port));
+    copy->ports = new t_port[pb_type->num_ports]();


Unsafe unless you make a default constructor for t_port, or you confirm we don't rely on one. Easier to make a default constructor, and useful.

vaughnbetz · 2025-05-16T21:24:23Z

libs/libarchfpga/src/arch_util.cpp

@@ -514,8 +513,7 @@ void alloc_and_load_default_child_for_pb_type(t_pb_type* pb_type,
        copy->ports[i].index = pb_type->ports[i].index;
        copy->ports[i].absolute_first_pin_index = pb_type->ports[i].absolute_first_pin_index;

-        copy->ports[i].port_power = (t_port_power*)vtr::calloc(1,
-                                                               sizeof(t_port_power));
+        copy->ports[i].port_power = new t_port_power();


Unsafe without a default constructor for t_port_power that sets everything to 0.

vaughnbetz · 2025-05-16T21:30:20Z

libs/libarchfpga/src/arch_util.cpp

-        copy->annotations[i].prop = (int*)vtr::malloc(sizeof(int) * pb_type->annotations[i].num_value_prop_pairs);
-        copy->annotations[i].value = (char**)vtr::malloc(sizeof(char*) * pb_type->annotations[i].num_value_prop_pairs);
+        copy->annotations[i].prop = new int[pb_type->annotations[i].num_value_prop_pairs];
+        copy->annotations[i].value = new char*[pb_type->annotations[i].num_value_prop_pairs];


If you follow @soheilshahrouz 's suggestion on a vector of pairs of ints/strings above this copy will become easier -- should turn into something like copy->annotations = pb_type->annotations since the vector/pair/string classes know how to properly do a deep-copy assignment.

vaughnbetz · 2025-05-16T21:30:52Z

libs/libarchfpga/src/arch_util.cpp

@@ -613,10 +610,9 @@ void ProcessLutClass(t_pb_type* lut_pb_type) {

    lut_pb_type->modes[0].interconnect[0].parent_mode_index = 0;
    lut_pb_type->modes[0].interconnect[0].parent_mode = &lut_pb_type->modes[0];
-    lut_pb_type->modes[0].interconnect[0].interconnect_power = (t_interconnect_power*)vtr::calloc(1, sizeof(t_interconnect_power));
+    lut_pb_type->modes[0].interconnect[0].interconnect_power = new t_interconnect_power();


Unsafe without a default constructor for t_interconnect_power

vaughnbetz · 2025-05-16T21:40:38Z

libs/libarchfpga/src/arch_util.cpp

@@ -944,7 +934,7 @@ void SyncModelsPbTypes_rec(t_arch* arch,

        pb_type->model_id = model_match_prim_id;
        vtr::t_linked_vptr* old = model_match_prim.pb_types;
-        model_match_prim.pb_types = (vtr::t_linked_vptr*)vtr::malloc(sizeof(vtr::t_linked_vptr));
+        model_match_prim.pb_types = new vtr::t_linked_vptr;


We could probably change this linked list (which currently stores pb_type* pointers) to a vector of pb_type* (or maybe even just to direct pb_type elements). Probably don't do that yet though ... we should refactor in stages.

vaughnbetz · 2025-05-16T21:40:52Z

libs/libarchfpga/src/read_fpga_interchange_arch.cpp

@@ -219,7 +219,7 @@ static t_port get_generic_port(t_arch* arch,
    port.is_non_clock_global = false;
    port.model_port = nullptr;
    port.port_class = vtr::strdup(nullptr);
-    port.port_power = (t_port_power*)vtr::calloc(1, sizeof(t_port_power));
+    port.port_power = new t_port_power();


calloc, need default constructor

vaughnbetz · 2025-05-16T21:41:15Z

libs/libarchfpga/src/read_fpga_interchange_arch.cpp

@@ -1293,7 +1293,7 @@ struct ArchReader {
        lut->model_id = get_model(arch_, LogicalModels::MODEL_NAMES);

        lut->num_ports = 2;
-        lut->ports = (t_port*)vtr::calloc(lut->num_ports, sizeof(t_port));
+        lut->ports = new t_port[lut->num_ports]();


calloc -> need t_port default constructor

vaughnbetz · 2025-05-16T21:41:44Z

libs/libarchfpga/src/read_xml_arch_file.cpp

@@ -517,10 +517,9 @@ void XmlReadArch(const char* ArchFile,
                /* This information still needs to be read, even if it is just
                 * thrown away.
                 */
-                t_power_arch* power_arch_fake = (t_power_arch*)vtr::calloc(1,
-                                                                           sizeof(t_power_arch));
+                t_power_arch* power_arch_fake = new t_power_arch();


calloc -> should make a default constructor for t_power_arch

vaughnbetz · 2025-05-16T21:42:10Z

libs/libarchfpga/src/read_xml_arch_file.cpp

@@ -533,11 +532,10 @@ void XmlReadArch(const char* ArchFile,
                /* This information still needs to be read, even if it is just
                 * thrown away.
                 */
-                t_clock_arch* clocks_fake = (t_clock_arch*)vtr::calloc(1,
-                                                                       sizeof(t_clock_arch));
+                t_clock_arch* clocks_fake = new t_clock_arch();


calloc -> need default constructor of t_clock_arch

vaughnbetz · 2025-05-16T21:50:44Z

It might be good enough to use = default to explicitly ask for default constructors, as I believe in that case all built-in type data members are initialized to 0 (e.g. int, float, etc.). That would only be a few lines of code, but you should test to be sure I'm right about the behaviour of that.
C++ has messed around a bit with the behaviour of constructors over the years, which is why the =default seems to change the behaviour of compiler-built default constructors.

soheilshahrouz · 2025-05-20T21:22:50Z

vpr/src/power/power.cpp

@@ -645,7 +645,7 @@ static void power_usage_blocks(t_power_usage* power_usage) {
 * Calculates the total power usage from the clock network
 */
 static void power_usage_clock(t_power_usage* power_usage,
-                              t_clock_arch* clock_arch) {
+                              std::vector<t_clock_network> clock_arch) {


You should probably pass this by reference as it is modifed by this function.

SamuelHo10 added 3 commits May 13, 2025 17:44

removed most mallocs

efd922c

Merge branch 'eliminate_free_and_malloc' of https://github.com/verilo…

2f679a7

…g-to-routing/vtr-verilog-to-routing into eliminate_free_and_malloc

replaced most of the callocs with news

0bdb49f

SamuelHo10 linked an issue May 14, 2025 that may be closed by this pull request

Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc #506

Open

github-actions bot added libarchfpga Library for handling FPGA Architecture descriptions lang-cpp C/C++ code labels May 14, 2025

SamuelHo10 added 2 commits May 14, 2025 11:50

formatted c++

7cb2095

undid a couple changes to fix odin memory leak

2c12600

SamuelHo10 requested review from amin1377 and vaughnbetz May 15, 2025 14:22

soheilshahrouz reviewed May 16, 2025

View reviewed changes

vaughnbetz requested changes May 16, 2025

View reviewed changes

SamuelHo10 force-pushed the eliminate_free_and_malloc branch from 82756a1 to 0031a71 Compare May 20, 2025 16:48

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label May 20, 2025

soheilshahrouz reviewed May 20, 2025

View reviewed changes

SamuelHo10 force-pushed the eliminate_free_and_malloc branch 2 times, most recently from 904a0cd to c0bf94c Compare May 21, 2025 17:59

SamuelHo10 changed the title ~~Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc~~ [WIP] Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc May 21, 2025

replaced t_clock_arch with std::shared_ptr<std::vector<t_clock_network>>

5be8c5d

SamuelHo10 force-pushed the eliminate_free_and_malloc branch from c0bf94c to 5be8c5d Compare May 21, 2025 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc #3040

[WIP] Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc #3040

SamuelHo10 commented May 14, 2025

soheilshahrouz left a comment

soheilshahrouz May 16, 2025

soheilshahrouz May 16, 2025

soheilshahrouz May 16, 2025

soheilshahrouz May 16, 2025

vaughnbetz left a comment

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz May 16, 2025

vaughnbetz commented May 16, 2025

soheilshahrouz May 20, 2025

[WIP] Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc #3040

Are you sure you want to change the base?

[WIP] Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc #3040

Conversation

SamuelHo10 commented May 14, 2025

soheilshahrouz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaughnbetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaughnbetz commented May 16, 2025

Choose a reason for hiding this comment