-
Notifications
You must be signed in to change notification settings - Fork 414
Route-verbosity #2723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Route-verbosity #2723
Conversation
Hi @AlexandreSinger, I have added route-verbosity, can you give this a quick look to double check if possible, I will also wait for the CI results. I can @ Professor Betz, when this is good and ready to be merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ZohairZaidi Thank you for looking into this issue. I have left some comments on how this code can be improved.
My main comment is that you should try to avoid passing around such a large data structure like router_opts
, when you really only need the route_verbosity
variable. It makes the code easier to read and understand. It can also make the code easier to modify in the future, in case someone decides to change how router_opts
is designed.
I also left some comments on some good practices when working on large code bases, such as passing large variables by reference and not injecting spaces when its not necessary. These issues may seem small, but if enough people do them the code can become unreadable and could cause bad performance degredations.
Thank you so much for the detailed feedback! I'm really happy to learn from your suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ZohairZaidi , just some minor comments that should be addressed. After this it should look good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Well done!
Hi @vaughnbetz, this is ready to merge. |
Thanks. Looks good to me! Merging. |
Thanks @vaughnbetz. I've encountered two types of warnings during the build process, and I'd like to ask for guidance on how to address them.
I would normally resolve these by initializing the variables with default values (e.g.,
Based on the information from Since the file is pre-generated and not dynamically regenerated during the We can either
Please let me know how you’d like me to proceed with both types of warnings! resolving these would result in a warning free build. |
@ZohairZaidi The output from VPR that Vaughn is referring to here is the runtime output of VPR, not the build time output. You are correct that we should aim to be warning clean at build time as well! The first warning is known. I think that file is part of Yosys. Yosys is an external tool that we use and we do not have direct access to (we would need to raise a PR on another repo and pull it in!). The second warning is one I have been trying to resolve for a little while, since it is a new warning in GCC-13 and is not allowing us to upgrade from GCC-11 to GCC-13 on the CI. I do not think we should modify this file. This is simply because if someone re-generates the file again it will trample your changes. We would need to fix this at its source. We would either need to fix the orginal source file which is being used to generate this file or we need to upgrade the tool itself (uxsdcxx). Another idea is that we can just disable this warning for this file only. But that would be last resort. |
I'm OK with disabling the warning for these files if we can find an easy way to do that, without modifying the files themselves (since one is autogenerated and the other we don't own). @ZohairZaidi : you could file an issue on the Yosys repo asking them to fix this warning, which would be the best solution for that one. You'd have to give the detailed warning/line and why we want it gone (to be warning clean). |
You would also need to be clear which version of GCC and Ubuntu you are using. |
Thank you both for the guidance! I will inquire about the yosys warnings and look into suppressing the -Wpedantic warning without modifying the file itself. I also intend to raise another issue about runtime output warnings soon. |
Added a new command-line option --route_verbosity to control the verbosity of the routing process output in VPR. Also added the correct command line options for this in the docs
Description
Related Issue
closes #2689
Motivation and Context
This change provides users with more flexibility in controlling the verbosity of the routing process.
How Has This Been Tested?
The --route_verbosity option has been tested on circuits to ensure that the correct amount of detail is output during the routing stage.
Types of changes
Checklist: