[Pack] Cleaned Up Read Netlist Code #3329
Conversation
The code for reading a netlist file was very hard to follow and made it challenging to try and improve the pb interface. Went through and cleaned up the code to make it a bit more obvious what it is trying to do.
github-actions
bot
added
VPR
vpr/src/base/read_netlist.cpp
Outdated
| if (hash_value->count == 1) { | ||
| VTR_ASSERT(*ncount == hash_value->index); | ||
| (*ncount)++; | ||
| processPb(child, index, new_child_pb, pb_route, num_primitives, loc_data); |
There was a problem hiding this comment.
I think we should refactor these functions to net be recursive. If we want to clean up the packer we really need to get rid of the recursions in these massive functions.
There was a problem hiding this comment.
This function is used for parsing XML. Unfortunately removing this recursion will be very challenging to do and may actually take away from the readability of this function. I think it will be best to leave this update behind for now. In the future when the t_pb type is cleaned up a bit more, I hope to absorb this method in a cleaner way.
| pin_node = alloc_and_load_port_pin_ptrs_from_string( | ||
| int* num_ptrs = nullptr; | ||
| int num_sets = 0; | ||
| t_pb_graph_pin*** pin_node = alloc_and_load_port_pin_ptrs_from_string( |
There was a problem hiding this comment.
Is this a 3D array or a 2D array of pointers? We should convert this to std::vector.
There was a problem hiding this comment.
This is a 2D array of pins. I also do not like this, but I think this will be better cleaned up in a future PR. This is drilled into how pb types are stored in VTR right now. I would prefer to leave it alone in this PR.
| const t_pb_type* child_pb_type = &(pb_type->modes[pb->mode].pb_type_children[i]); | ||
| for (int j = 0; j < child_pb_type->num_pb; j++) { | ||
| if (pb->child_pbs[i][j].name != nullptr) { | ||
| const_gen_count += mark_constant_generators_rec(&(pb->child_pbs[i][j]), pb_route, verbosity); |
There was a problem hiding this comment.
Same as above. It's incredibly hard to understand how exactly these functions work with the recursion.
There was a problem hiding this comment.
I also agree that recursion can make code a bit challenging to work with; however, the way that the pb_types are currently designed, recursion is the cleanest way of iterating over them. I hope to remove this limitation in a future PR.
|
@AmirhosseinPoolad I have resolve your comments, and left a note for any comments I do not think should change. Please let me know if you have any further comments. |
|
Thanks for the feedback @soheilshahrouz , resolved. Please let me know if you have any further comments. |
3 participants
The code for reading a netlist file was very hard to follow and made it challenging to try and improve the pb interface. Went through and cleaned up the code to make it a bit more obvious what it is trying to do.