vtr-verilog-to-routing
vtr-verilog-to-routing copied to clipboard
verilog-to-routing
Remaining malloc to new
Description
Changed remaining malloc/calloc/reallocs to new.
How Has This Been Tested?
Passes all CI tests.
Types of changes
- [ X] Bug fix (change which fixes an issue)
- [ ] New feature (change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Checklist:
- [ ] My change requires a change to the documentation
- [ ] I have updated the documentation accordingly
- [ ] I have added tests to cover my changes
- [X] All new and existing tests passed
document build failure is infrastructure issue so can remove draft. Are all free/malloc/calloc/realloc gone from related libraries like libarchfpga? Can you test runtime with QoR run on titan or vtr benchmarks? See https://docs.verilogtorouting.org/en/latest/README.developers/?highlight=QoR%20#collecting-qor-measurements for procedure.
All looks good except placed_wirelength_est looks too good (predicting a 6% reduction in wirelength, which shouldn't happen). Is that one a typo?
Looked at the results some more. Everything looks good except placed_wirelength_est looks too good in both comparisons. Was there a change in the master code? Is there a bug introduced in that computation (but nothing else, which would mean it is a bug in the bounding box -> wirelength estimation correction factors probably)? That code does have an array used for lookup and it might be 1-indexed (shifted pointer) so maybe it got messed up? Messing up that code wouldn't affect much or anything else other than that wirelength estimate, so that's a possible root cause.
Filing an issue on the parsing qor not working directly with the downloaded CI artifacts is a good idea. If you know how to fix the scripts go ahead and fix them; otherwise we can probably get Jeff G. or Seyed to fix.
In the artifacts of GitHub's tests, every placed_wirelength_est value is -1... (see below)
I've checked over multiple tests and multiple actions, and they're all -1. I think I need a second set of eyes on this to make sure I'm not going insane.
Parsing QoR results doesn't work with parse_results produced when running tests from the command line (i.e. results produced from ./run_reg_test.py vtr_reg_XYZ) either. I've made an Issue (Issue #2107 ) & PR (PR #2108 ) to address this.
Not sure about the -1 place_wirelength_est in the github artifacts; maybe not parsed out in the CI tests? The regression and CI scripts and conf files are fairly involved and have the ability to specify what gets parsed out.
It turns out the QoR results I included in my wiki were wrong-- I editted the files manually, and messed up so that I shifted the results over by 1 column. My QoR_compare.py script edit is actually correct, and placed_wirelength_est is -1. Manually converting the .txt files to a spreadsheet shows placed_wirelength_est is -1 as well. I don't know what to do, so I'll send an email with relavent files.
Thanks @hzeller . Good suggestions. @jmah76 : if placed_wirelength_est is -1 in the master and your code, then it is fine. If it is -1 in your code but not master, you should rebase this PR onto (or merge in) the latest master so you get the updated placed_wirelength_est code. If everything else matches there is almost certainly no issue in any case though.
If you update your spreadsheets, please attach them here so we have a record of them (better than emailing them).
Getting close: please confirm all review suggestions addressed (and push latest code), resolve conflicts, and upload the latest QoR spreadsheet and summarize. Then we can merge.
Looks like there are a few errors in the QoR tests -- I think results were not generated for a few runs, like: 00:13:31 | Warning: task includes result for arch.timing.xml/picosoc_basys3_full_100.eblif missing in golden results 00:13:31 | Warning: task includes result for arch.timing.xml/picosoc_basys3_full_50.eblif missing in golden results 00:13:31 | Warning: task includes result for arch.timing.xml/linux_arty.eblif missing in golden results 00:13:31 | Warning: task includes result for arch.timing.xml/minilitex_arty.eblif missing in golden results 00:13:31 | Warning: task includes result for arch.timing.xml/minilitex_ddr_arty.eblif missing in golden results 00:13:31 | Warning: task includes result for arch.timing.xml/minilitex_ddr_eth_arty.eblif missing in golden results
@jmah76 : can you take a look? Not sure why warnings passed on this one, and not on Sebastian's PR (maybe he needs to update the branch)?
6 failures (missing QoR data?) in nightly_test1. One slightly higher vpr_max_mem on a small design in nightly_test2: stratixiv_arch.timing.xml/murax_stratixiv_arch_timing.blif/common max_vpr_mem relative value 1.202197661475418 outside of range [0.8,1.2], above absolute threshold 102400.0 and not equal to golden value: 812136.0
The nightly test 2 failure is spurious; you can just update the golden results to have the new memory footprint number (it's not a big change -- just slightly more than the allowed 20%).
It's not missing QoR data-- one of the ASSERTs in bucket.cpp is failing because of the shift from using a vector to temporarily store the heap_ array for the resize to changing the heap_ array to a vector. I don't know why this is happening.
If you can't sort it you can revert that one (just go back). If you look at it closely hopefully you can figure out the bug, but if not, revert.
The bucket heap is only used for the Artix7 (Symbiflow) compiles, so it's not really the default flow.
I reverted only bucket.cpp back, and vtr_reg_nightly 1 seems to be working now. If it's okay, to fix vtr_neg_nightly 2, I'll do what you suggested here:
The nightly test 2 failure is spurious; you can just update the golden results to have the new memory footprint number (it's not a big change -- just slightly more than the allowed 20%).
CI passed. @jmah76 : can you attach the QoR comparison data so we see all is well (should be, but I'd like to check). There was some earlier data attached but I believe it had parse errors and wasn't correct.
QoR generally looks good (unchanged critical paths, wirelength etc. on the vtr benchmarks, and roughly tied cpu time). Memory footprint is unchanged on the larger designs, but goes up by 1.6x or even 2.2x on the smaller designs; suspect some small data structures is consuming more memory due to being turned into a vector but it has no impact on larger designs since not dominant / at peak.
To be careful @jmah76 will attach the vtr QoR spreadsheet and run and attach the Titan one.
@vaughnbetz Full Titan QoR results are on my wiki. Nothing looks concerning...
Full on my wiki.