Skip to content

Search Update + NoC GUI Update #2102

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 8 commits into from
Aug 4, 2022
Merged

Search Update + NoC GUI Update #2102

merged 8 commits into from
Aug 4, 2022

Conversation

SebastianLievano
Copy link
Contributor

**Search **
Updated search functionality to include Block and Net Name suggestions/autocomplete, toggleable by hitting the "Enter" or "Return" key.
Additionally, switched search to include Cluster and Atom netlist for Blocks, and the Atom netlist for nets (Cluster netlist is a subset of atom). The autosuggest algorithm/set up is meant to only run once when you hit enter, so you may need to rehit the key to reactivate it. Upon hitting enter, the program will search through all of the names and display all of the names that contain the given substring. This is obviously a large operation, and will take a bit of time.

Furthermore, searching a subblock now highlights the block if it is visible at the current zoom lvl, and if not, highlights the containing cluster block. Additionally, a new subroutine which highlights an Atom Block from its id has been added, could be useful.

NoC Graphics Additions
Added an NoC graphics combo box in the Block Settings dropdown. This will only display if the user uses the "--noc on" option on startup.

Motivation and Context

The original search function had no form of automatic completion, required users to enter the full names of the block or net they were searching for, with no room for error, and was also fully based on the Clustered netlist, and thus could not search or highlight internals. It was pretty difficult to use as a result, as you'd need to know the full name of the thing you were searching for, and the names are often synthesized by VTR so you probably would not know that.

The new functionality will automatically parse through all names to find those that contain the given string. This allows users to search for a specific string even if they don't know its full name, and they can enable auto-suggestion only when they need it by pressing enter.

How Has This Been Tested?

All new GUI features have been tested and ensured to perform as expected. Search functionality has also been tested a lot on different architectures of varying sizes.

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)

@github-actions github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Jul 21, 2022
@vaughnbetz
Copy link
Contributor

  1. Please add cpu time data on a couple of cases for posterity.
  2. The build the docs error is an unrelated infrastructure issue so can ignore.

@vaughnbetz
Copy link
Contributor

Needs a make format too.
@duck2 : may need this PR as he is hitting net search issues in the old UI.

@vaughnbetz
Copy link
Contributor

Read the docs error was an infrastructure issue (now fixed). Can ignore.
Need to do a 'make format' though

Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

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

Some commenting changes suggested and a few minor restructurings. Looks very good and clean overall!

if (type && type[0] == '\0') {
warning_dialog_box("Please select a search type");
app->refresh_drawing();
return "Failed lol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is the best failure code ... empty string instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh oops forgot to get rid of this. Set it to empty string and added checks where it is called

@vaughnbetz
Copy link
Contributor

One warning in vpr that should be fixed.
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/draw/search_bar.cpp:372:14: error: unused variable ‘tptr’ [-Werror=unused-variable]
372 | t_trace* tptr;

@vaughnbetz
Copy link
Contributor

@jmah76 @SebastianLievano : this looks good to merge except the "error on warning" compiler check is failing for vpr.
One warning is in Sebtasian's new code (listed above, get rid of an unused tptr).
A bunch are warnings on code of the type
delete (pin_criticality+1)
Those are likely cropping up due to the free->delete changes. We probably suppressed those warnings with free (you can #pragma off specific warnings in specific files) but haven't suppressed the delete versions. Jennifer, can you either suppress them or (even better!) just allocate one more entry in the appropriate array (e.g. pin_criticality) and get rid of the pointer adjustment (e.g. pin_criticality--) and the +1 in the delete call. Maybe you've already done that and we just need to do an update branch operation?

@jmah76
Copy link
Contributor

jmah76 commented Aug 2, 2022

I think the pin_criticality + 1 warnings were there before I started (not 100% sure, but I haven't touched route_timing.cpp, the file where the warnings are popping up). Those "errors" are suppressed, but I can try to look into this on a different branch (branch route_timing_offset)

@vaughnbetz
Copy link
Contributor

OK, if those errors are suppressed and the test passes once the tptr error is gone, I'm OK with it. Getting rid of them would be even cleaner if you can though! If you expand the array/vector to have a 0'th entry, please set that 0'th entry to invalid values (NULL for a pointer, -1 for an integer, etc.) so we are likely to crash if we accidentally use it.

@jmah76
Copy link
Contributor

jmah76 commented Aug 2, 2022

I made the changes here. I was able to get rid of the warnings by adding an entry in the arrays that were throwing the warnings. Their "zeroth" elements are either -1 (for the float and int arrays) or nullptr (for the ptr arrays). Once the CI tests pass, I'll make a PR.

@vaughnbetz
Copy link
Contributor

Great, thanks Jennifer.

@SebastianLievano
Copy link
Contributor Author

Added final modifications to toggle completion off after first showing. However, some limitation either in EZGL or GTK (probably some bug with focuses) means some key presses do not register in the function, making the toggling a bit inconsistent. Not worth pursuing imho. Will merge once checks are done @vaughnbetz

@vaughnbetz
Copy link
Contributor

One failure, but looks like an unrelated infrastructure issue. Can you rerun that test and see if it passes now (instructions are in the developer guide in the web documentation)?

@vaughnbetz vaughnbetz merged commit a80248d into master Aug 4, 2022
@vaughnbetz vaughnbetz deleted the Seb_Search branch August 4, 2022 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants