[AP][Legalizer] Added Ability to Generate a Mass Report #3095
Conversation
github-actions
bot
added
VPR
|
@amin1377 I am not sure how much use you will get out of this report, but I think it wouldn't hurt! Can you please review when you have a moment? |
AlexandreSinger
force-pushed
the
feature-ap-mass-report
branch
from
d651165 to
4f8e10f
Compare
While working on the mass abstraction in the partial legalizer of the globlal placer, found that I needed a lot more information on the device to be able to debug the mass calculations. Added a command-line option which will generate a mass report if requested. This mass report contains useful information on the device, the netlist, and the mass / capacities computed in the mass calculator.
AlexandreSinger
force-pushed
the
feature-ap-mass-report
branch
from
4f8e10f to
39ad2b7
Compare
There was a problem hiding this comment.
Thanks, Alex! As we discussed before, I really liked the idea of generating this report. I’ve added a bunch of small comments.
The most important one is that generating this report; at least the parts based solely on the architecture file shouldn’t be made AP-specific. For the other parts (like generating the mass report), it would probably be more challenging, since that would require adding the capability to calculate mass for the placed atom netlist, which is likely beyond the scope of this PR.
| @@ -1336,6 +1336,15 @@ Analytical Placement is generally split into three stages: | |||
|
|
|||
| **Default:** ``1`` | |||
|
|
|||
| .. option:: --ap_generate_mass_report {on | off} | |||
There was a problem hiding this comment.
I looked at the report and noticed that there is no AP-specific section in it. So, I think it might be better to change the name of the option to avoid implying that it's AP-specific, and instead treat it as one of the general options for generating a report.
There was a problem hiding this comment.
The mass calculation is AP specific, the report cannot be generated outside of the AP flow. The drawing of the complex graph is useful for understanding the mass report, hence why it is part of this report.
I do not want to gate this PR on this, but in a future PR I can move this to an external report which reports on the structure of the architecture file. I think that is beyond the scope of this PR and would include more features. What do you think?
There was a problem hiding this comment.
I agree. Let's do it in another PR.
vpr/src/base/ShowSetup.cpp
Outdated
| @@ -657,6 +657,7 @@ static void ShowAnalyticalPlacerOpts(const t_ap_opts& APOpts) { | |||
|
|
|||
| VTR_LOG("AnalyticalPlacerOpts.ap_timing_tradeoff: %f\n", APOpts.ap_timing_tradeoff); | |||
| VTR_LOG("AnalyticalPlacerOpts.log_verbosity: %d\n", APOpts.log_verbosity); | |||
| VTR_LOG("AnalyticalPlacerOpts.generate_mass_report: %s\n", APOpts.generate_mass_report ? "true" : "false"); | |||
There was a problem hiding this comment.
I'm not sure what the convention is for displaying parameter values in the vpr.out report. I think it would be better to avoid printing all parameter values and instead only include those that are necessary for analyzing the results. If you agree, I suggest we omit the value of the generate_mass_report parameter (or any similar parameter that simply controls whether a report should be generated).
There was a problem hiding this comment.
I think thats a fair point. I am actually not sure the convention, but I have just been adding all options that the user would pass in. This provides feedback that their input was observed.
| #include "vtr_time.h" | ||
| #include "vtr_vector.h" | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
Just out of curiosity: I think the main reason for using an anonymous namespace here is because of the structs. But if the structs weren't present, is there any real difference between using static functions and defining functions in an anonymous namespace?
There was a problem hiding this comment.
There should be no difference between labelling functions as static or putting them into the anonymous namespace. Generally I prefer using the static keyword, but if an anonymous namespace already exists I like to put them in there.
| * @param prefix | ||
| * The prefix that all children of this node will print before their names. | ||
| */ | ||
| void print_tree_node(const PrintingTreeNode& node, |
There was a problem hiding this comment.
I'm not sure whether it's a common convention or not, but in other parts of VTR, the "recur" suffix is added to the names of recursive functions.
While working on the mass abstraction in the partial legalizer of the globlal placer, found that I needed a lot more information on the device to be able to debug the mass calculations.
Added a command-line option which will generate a mass report if requested. This mass report contains useful information on the device, the netlist, and the mass / capacities computed in the mass calculator.
Example reports:
Notably, I added the ability to print the complex block graph; which is extremely useful for debugging the mass calculator: