vtr-verilog-to-routing icon indicating copy to clipboard operation
vtr-verilog-to-routing copied to clipboard

[ClusterLegalizer] Code Cleanups

Open AlexandreSinger opened this issue 5 months ago • 0 comments

After cleaning up the ClusterLegalizer code into an API, there are still some small cleanups which are not required for operation but may improve the readability of the code or improve the performance of clustering (reduce memory usage, reduce runtime, improve QoR).

  • [ ] The ClusterLegalizer now maintains a count of the number of clusters that currently exist. There was a variable that was left behind in the clustering code named total_clb_num which can probably be removed: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/e38c0f3d975decbafc112d18877a99506c7e845f/vpr/src/pack/cluster.cpp#L387-L390
  • [ ] The constructor of the ClusterLegalizer is very long due to a lot of integer variables being passed in which are only used to allocate C-style dynamic arrays (the original author allocated arrays to a max size). These dynamic arrays should instead be implemented as std::vectors which would be more space efficient and maybe even more performant. Specifically feasible_block_array_size which is a historical barnacle which may no longer be needed. https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/e38c0f3d975decbafc112d18877a99506c7e845f/vpr/src/pack/cluster_legalizer.h#L212-L239
  • [ ] Pack molecules have a flag in their data structure called valid, which signifies that a molecule has been unclustered. The naming of this variable is very confusing and this flag is really not needed. The ClusterLegalizer has a method called is_atom_clustered which can just check if any of the atoms in the molecule have been clustered (if a molecule has been clustered, all atoms in that molecule should be clustered). This will make the code more readable and would prevent the ClusterLegalizer from modifying the pack molecules themselves (which really should not change during clustering). https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/e38c0f3d975decbafc112d18877a99506c7e845f/vpr/src/pack/cluster_legalizer.cpp#L1369-L1375
  • [ ] The try_pack_molecule method within the ClusterLegalizer is very long and complicated. This method should be split into multiple helper methods. This may also be a good opportunity to organize this method in a more clean way that works well with the ClusterLegalizationStrategy. https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/e38c0f3d975decbafc112d18877a99506c7e845f/vpr/src/pack/cluster_legalizer.cpp#L1137-L1140
  • [ ] There are global memory accesses in the ClusterLegalizer which are used for storage which should be cleaned up and stored within the class itself. Most notably is the atom pb lookup in the atom_ctx which is used to store the primitive pb of the given atom. This creates a major limitation of the ClusterLegalizer where it cannot exist at the same time as the ClusteredNetlist and 2 ClusterLegalizers cannot exist at the same time. Instead, this lookup should be stored in the ClusteredNetlist class itself. Removing this global access would make the class significantly more usable. https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/e38c0f3d975decbafc112d18877a99506c7e845f/vpr/src/pack/cluster_legalizer.cpp#L577-L582
  • [ ] There is currently state leaking out from the ClusterLegalizer from the cluster's pb. Each cluster maintains its own pb during clustering which it frees after the cluster is destroyed. These pbs are being leaked out from the cluster legalizer and used outside of it. This can be unsafe since the user may make changes to these pbs which the ClusterLegalizer is not aware of. These should be changed to some form of accessor methods which manipulate / read the pbs. https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/e38c0f3d975decbafc112d18877a99506c7e845f/vpr/src/pack/cluster_legalizer.h#L380-L385

AlexandreSinger avatar Sep 20 '24 14:09 AlexandreSinger