[Infra] Cleaned Up Include Files in VPR Base Directory #3051
Conversation
github-actions
bot
added
VPR
Many include files in the base directory contained includes to other headers which they do not use. This causes many CPP files to include way more header files than they need, increasing the incremental build time. This process needs to be done on the entire VTR repo, but I found that the base directory was one of the biggest culprits of this and the hardest to untangle.
AlexandreSinger
force-pushed
the
feature-include-cleanup
branch
from
99fc203 to
ced55e7
Compare
There was a problem hiding this comment.
Thanks @AlexandreSinger, I didn't think this could actually happen this quick, nice job. I only have one concern, is the UNDEFINED in librrgraph same as the UNDEFINED in libarchfpga? There's also a place where UNDEFINED is used as an index which is extremely unsafe (and I'm also not sure why it works at all). But if you don't feel like going through that now, it's fine by me to file an issue for it and fix it in a later PR. Other than that, LGTM.
| @@ -482,7 +482,7 @@ size_t t_rr_graph_storage::count_rr_switches( | |||
| for(size_t iswitch = 0; iswitch < arch_switch_counts.size(); ++iswitch) { | |||
| if(arch_switch_fanins[iswitch].empty()){ | |||
| if(arch_switch_inf[iswitch].fixed_Tdel()){ | |||
| arch_switch_fanins[iswitch][UNDEFINED] = num_rr_switches++; | |||
| arch_switch_fanins[iswitch][ARCH_FPGA_UNDEFINED_VAL] = num_rr_switches++; | |||
There was a problem hiding this comment.
Unrelated to this PR: Is this a -1 index to an array? Why does this not crash when iswitch == 0? We should add an issue for this.
There was a problem hiding this comment.
Hi @AmirhosseinPoolad thanks for the review! I saw this too. After I quickly investigated this I found that arch_switch_fanins is a vector of maps:
So using -1 is valid in this case. This is not the cleanest thing to do since it causes confusion, but I think this is fine.
There was a problem hiding this comment.
Also, regarding if librrgraph uses the same UNDEFINED as libarchfpga, librrgraph does not have its own UNDEFINED. It appeared to always use libarchfpga's UNDEFINED (as far as I can tell). So I gave it to them. However, ideally librrgraph should have its own undefined to prevent issues.
I had to rename the libarchfpga UNDEFINED since it conflicts with vpr's UNDEFINED (even though they have the same value.
There was a problem hiding this comment.
Also, FYI, this is not all cases of bad includes; these are just the ones I have been eying up for the last few months that have been bothering me. The base/ folder is used throughout VPR, so cleaning it up the header files in this directory should deal with some of the worst offenders.
Many include files in the base directory contained includes to other headers which they do not use. This causes many CPP files to include way more header files than they need, increasing the incremental build time.
This process needs to be done on the entire VTR repo, but I found that the base directory was one of the biggest culprits of this and the hardest to untangle.
See Issue #624 and PR #3048 for context.