verilog-to-routing / vtr-verilog-to-routing

Verilog to Routing -- Open Source CAD Flow for FPGA Research
https://verilogtorouting.org
Other
999 stars 385 forks source link

[Netlist] Potential Issue When Re-Creating Elements in Atom and Clustered Netlists #2690

Open AlexandreSinger opened 4 weeks ago

AlexandreSinger commented 4 weeks ago

When implementing another Netlist class based on the Atom and Clustered netlists, I noticed a bug in their create methods.

This bug occurs in a few places, but for example the create block method specifies that it creates a block or returns the ID if it already exists: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/21d015036969a955298cbc9b94ca74fd35a016be/vpr/src/base/atom_netlist.h#L167-L175

However, in the implementation, the push_back method is used on the internalized data: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/21d015036969a955298cbc9b94ca74fd35a016be/vpr/src/base/atom_netlist.cpp#L140-L155

Netlist::create_block will return the ID of a block if one with that name already exists; but then there will be a mismatch with the internal data. This will cause a crash if a block is re-created into the netlist.

This can be simply fixed by changing push_back into insert. The performance should not be too different since the vtr::vector_map class treats insertions as a resize (similar to a push back).

This should be fixed for many of the create methods in both the Atom Netlist and the Clustered Netlist (or the documentation should be updated to reflect that re-creation of IDs is not supported).

AlexandreSinger commented 3 weeks ago

@ZohairZaidi This is a good coding issue that you may be able to help with. In the Atom and Clustered netlists, I noticed a bug with how they maintain their internal data. I was wondering if you wanted to take a crack at this issue? It should be relatively simple, it would just need to be tested through our CI to ensure nothing breaks.

This issue can be found in the "create_..." methods in the following two files:

Feel free to let me know if you have any questions!

CC: @vaughnbetz

ZohairZaidi commented 3 weeks ago

Thanks @AlexandreSinger I just submitted a PR. Ran some local tests but will wait for the CI results as well. Please feel free to point out anything I may have done wrong or if you would like it a different way.