Skip to content

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

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Conversation

ZohairZaidi
Copy link
Contributor

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

  • 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
  • All new and existing tests passed

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool docs Documentation lang-cpp C/C++ code labels Sep 14, 2024
@ZohairZaidi
Copy link
Contributor Author

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

Copy link
Contributor

@AlexandreSinger AlexandreSinger left a 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.

@ZohairZaidi
Copy link
Contributor Author

Thank you so much for the detailed feedback! I'm really happy to learn from your suggestions

Copy link
Contributor

@AlexandreSinger AlexandreSinger left a 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!

Copy link
Contributor

@AlexandreSinger AlexandreSinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Well done!

@ZohairZaidi
Copy link
Contributor Author

Hi @vaughnbetz, this is ready to merge.

@vaughnbetz
Copy link
Contributor

Thanks. Looks good to me! Merging.
@AlexandreSinger : I wonder if we should do something similar for other phases (clustering, placement)?
Another useful follow-up: we should take a look at the output from VPR and make sure we're not printing any spurious warnings or excessive (developer-focused) output by default.

@ZohairZaidi
Copy link
Contributor Author

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.

  1. Uninitialized Variable Warnings (-Wmaybe-uninitialized):
    For instance, in satgen.cc, the compiler is warning that variables like undef_srst and undef_ce may be used uninitialized:

I would normally resolve these by initializing the variables with default values (e.g., 0 ). Should we proceed with this approach, or is there a more preferred way to you'd recommend handling these cases?

  1. Extra Semicolon Warnings (-Wpedantic):
    I’m also seeing warnings related to extra semicolons in the generated rr_graph_uxsdcxx.capnp.c++ file:

Based on the information from ReadME.gen.md, this file is pre-generated via uxsdcxx from a .capnp schema file and is checked into the repository. This is done to avoid requiring Python3 and other dependencies to regenerate the file during the build process.

Since the file is pre-generated and not dynamically regenerated during the make process, I believe it should be safe to modify the file directly. However, I wanted to double-check and confirm if it’s okay to make changes directly to this file, or if there's a particular reason we should avoid modifying it manually.

We can either

  1. Modify the File
  2. Suppress the Warning

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.

@vaughnbetz vaughnbetz merged commit d554ff9 into verilog-to-routing:master Sep 18, 2024
36 of 52 checks passed
@AlexandreSinger
Copy link
Contributor

@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.

@vaughnbetz
Copy link
Contributor

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).

@AlexandreSinger
Copy link
Contributor

You would also need to be clear which version of GCC and Ubuntu you are using.

@ZohairZaidi
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation lang-cpp C/C++ code VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logging] Overly Verbose Log Messages When Building RRGraph
3 participants