Pass actual fc instead of fc_max to connect to pins #2661
Conversation
…c fc instead of maximum fc
…rilog-to-routing into pass_actual_fc_to_connect_to_pins
…rilog-to-routing into pass_actual_fc_to_connect_to_pins
github-actions
bot
added
VPR
saaramahmoudi
changed the title
|
These links are the results for this branch for VTR and Titan benchmarks: |
|
A few QoR failures; they seem to be higher routing areas, maybe expected due to the Fc handling change. I checked vtr_reg_basic and it is routing area increases (37%, above the 30% tolerance) but only on the diffeq circuit (which is small). That seems likely to be OK and just needing a golden result update; other failures are hopefully similar. |
There was a problem hiding this comment.
Looks good; I just suggest some Doxygen comments for the modified routines (which didn't have them before, but adding them would make you a force for good @saaramahmoudi !).
|
QoR looks good; roughly a tie for VTR and actually a bit better for Titan. |
The spreadsheet I attached for Titan has the area per tile for all circuits (parse_results_master.txt and parse_results.txt tabs). Geomean averaged over all circuits; the routing area per tile for the master branch is "18519," and for my branch, it is "18518", which confirms that we are getting a close number in both branches. |
|
Thanks, that looks good. |
|
If you're able to add the requested comments we can merge. If you can't add those in the near future we could merge too (although it would be nice to get them in while we're looking at the code). |
I am adding the comments right now. After I push to the branch with updated golden results, I will ping you. |
…m/verilog-to-routing/vtr-verilog-to-routing into pass_actual_fc_to_connect_to_pins
vpr/src/route/rr_graph.cpp
Outdated
| bool channel_widths_unchanged(const t_chan_width& current, const t_chan_width& proposed); | ||
|
|
||
| /** | ||
| * @brief This routine calculate pin connections to tracks for a specific type on the Fc value defined for each pin in the architecture file. |
There was a problem hiding this comment.
What is the difference between this routine and the one below it? From their comments they both seem to do the same thing. One returns a 5D matrix of vectors, and the other a 6D matrix, but other than that they don't seem different. Could one be deleted (don't worry about it if that is too much work, but we could put a TODO in if that's the better long term path).
There was a problem hiding this comment.
The one that returns the 6D matrix is written as a helper function for the other one. Basically, we first calculate the number of connections should be made for each pin for the current tile and save a relative track indices. Then, we return the 6D matrix and post process it to calculate the absolute track number and keep a vector of connection per each pin. Note that basically they return the same thing (6D matrix of int and 5D matrix of integer vector) but the logic within each function are different.
|
@vaughnbetz applied requested changes, ready to merge! |
|
@vaughnbetz This PR failed the strong tests. After merging, master is now failing the strong tests... |
|
@saaramahmoudi Do you think this is a problem? Or do you think the golden solutions just need to be regenerated? |
|
@AlexandreSinger @vaughnbetz I think we just need to generate the golden results for those. |
|
@saaramahmoudi Perfect! Thats good news. Would you be able to do that in a PR? If not I can raise a PR tomorrow if I find time. |
|
@AlexandreSinger will raise a PR tomorrow! |
Description
This PR addresses the issue mentioned in Issue #2641.
Motivation and Context
How Has This Been Tested?
I tested the branch with my examples with fc_override with different pins. @WhiteNinjaZ has also tested the branch on a couple more benchmarks and confirmed that it works properly.
Types of changes
QoR:
We expect the RR graph to change based on this change. Thus, I have initiated Titan and VTR benchmarks to compare QoR with the master branch. Will attach the results as soon as it is ready.