Skip to content

Deploy new API rr_node.node_sides() to GUI #1661

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

Conversation

tangxifan
Copy link
Contributor

@tangxifan tangxifan commented Feb 11, 2021

This is a follow up pull request for the PR #1621
The purpose of this PR is to deploy the new APIs about the node sides of RRGraph to the Graph User Interface (GUI)

Note
This PR will not deprecate the old APIs and the unnecessary node_sides() APIs.
The deprecation will be done after lookahead map has been fixed, as the final PR in this series of patches

Description

Major changes are landed in the GUI part
In the GUI part, the major changes to enable the drawing for lines between the IPIN/OPIN and CHANX/CHANY routing resource nodes.
As an IPIN/OPIN may have multiple sides, the drawer should identify the correct line (from the multiple possible connections) to display.

** A Side Note**
A separated PR will be created to solve the issue on the router lookahead map (as stated in issue #1580)
In the router lookahead map, a proper side should be picked from multiple sides of a rr_node so that the lookahead map can have an accurate estimation on the routing congestion.
As multiple sides are used in Titan architectures, a proof of the QoR changes is required!

Related Issue

To address issue #1577

Motivation and Context

To show correct RRGraph display for nodes/edges with multiple sides in GUI.

How Has This Been Tested?

As multiple sides are used in Titan architectures, a proof of the GUI display is required!

My major focus is on the Titan architecture where multiple sides are heavily used for rr_nodes.

  • Before GUI patching (LAB in Titan Architecture):
    image

  • After GUI patching (LAB in Titan Architecture): You can see the connections on the top side has been correctly shown, which are driven by LAB.data_in (locate on TOP, LEFT, RIGHT sides)
    image

  • Before GUI patching (PLL and I/Os in Titan Architecture):
    image

  • After GUI patching (PLL and I/Os in Titan Architecture):
    image

  • Before GUI patching (M9K in Titan Architecture):
    image

  • After GUI patching (M9K in Titan Architecture):
    image

  • Before GUI patching (DSP36 in Titan Architecture):
    image

  • After GUI patching (DSP36 in Titan Architecture):
    image

**Note that there is no difference on normal architectures.

  • Before GUI patching (k6_N10_mem32K_40nm.xml):
    image

  • After GUI patching (k6_N10_mem32K_40nm.xml):
    image

The changes will only affect GUI codes under vpr/src/draw.

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 the VPR VPR FPGA Placement & Routing Tool label Feb 11, 2021
@tangxifan tangxifan changed the title [WIP] Deploy new API rr_node.node_sides() to Client Functions [WIP] Deploy new API rr_node.node_sides() to GUI Feb 11, 2021
@tangxifan
Copy link
Contributor Author

@vaughnbetz @litghost I have finished my patch on the GUI using the new node_side() APIs.
All the graphic proofs have been uploaded. This PR is ready for your review.

@tangxifan tangxifan changed the title [WIP] Deploy new API rr_node.node_sides() to GUI Deploy new API rr_node.node_sides() to GUI Feb 13, 2021

float draw_pin_offset;
if (pin_rr.side() == TOP || pin_rr.side() == RIGHT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this logic needs a comment. Now that pin rr nodes have multiple edges, I don't think this logic is right. I recommend you write a short comment explaining why you believe this is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. In the later part of this code block, I have logics to identify which side to be used for GUI. Therefore, I move the offset calculation there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed: good to comment why the offset is +ve or -ve (draw above / right, or draw below/left --> latter implies a negative offset).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vaughnbetz No problem. I have added comments about why the positive/negative offset is needed

Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM. Only had one suggestion for a comment (same place Keith flagged).


float draw_pin_offset;
if (pin_rr.side() == TOP || pin_rr.side() == RIGHT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed: good to comment why the offset is +ve or -ve (draw above / right, or draw below/left --> latter implies a negative offset).

@vaughnbetz
Copy link
Contributor

This is all graphics, so it won't affect the nightly tests. Will merge as soon as sanitized complete, although I think this code is not active in them either.

@vaughnbetz vaughnbetz merged commit 4b12c57 into verilog-to-routing:master Mar 3, 2021
@tangxifan tangxifan mentioned this pull request Mar 24, 2021
9 tasks
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