Eliminate vtr_free and vtr_malloc/vtr_calloc/vtr_realloc #3040
Conversation
github-actions
bot
added
libarchfpga
There was a problem hiding this comment.
Thanks @SamuelHo10
I left a few comments suggesting how the code can be improved in future PRs.
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
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.
|
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. |
SamuelHo10
force-pushed
the
eliminate_free_and_malloc
branch
from
82756a1 to
0031a71
Compare
SamuelHo10
force-pushed
the
eliminate_free_and_malloc
branch
from
0031a71 to
904a0cd
Compare
SamuelHo10
force-pushed
the
eliminate_free_and_malloc
branch
from
904a0cd to
c0bf94c
Compare
SamuelHo10
changed the title
SamuelHo10
force-pushed
the
eliminate_free_and_malloc
branch
2 times, most recently
from
f11da67 to
b697f60
Compare
SamuelHo10
force-pushed
the
eliminate_free_and_malloc
branch
from
b697f60 to
e7b470a
Compare
| @@ -2202,7 +2200,8 @@ struct t_arch { | |||
| LogicalModels models; | |||
|
|
|||
| t_power_arch* power = nullptr; | |||
| t_clock_arch* clocks = nullptr; | |||
|
|
|||
| std::shared_ptr<std::vector<t_clock_network>> clocks; | |||
There was a problem hiding this comment.
Why is this a shared_ptr? Do we have multiple copies of this pointer? If not, I think unique_ptr would be a better choice.
There was a problem hiding this comment.
It is shared by device_ctx.clock_arch. I think @vaughnbetz mentioned that in the future, we should get rid of the pointer entirely by assigning the value to device_ctx.clock_arch at the end. Currently, the code is creating two pointers with the same memory address and assigning a value after which can be dangerous.
SamuelHo10
changed the title
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.