-
Notifications
You must be signed in to change notification settings - Fork 415
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
Deploy new API rr_node.node_sides() to GUI #1661
Conversation
@vaughnbetz @litghost I have finished my patch on the GUI using the new node_side() APIs. |
|
||
float draw_pin_offset; | ||
if (pin_rr.side() == TOP || pin_rr.side() == RIGHT) { |
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.
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.
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.
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.
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.
Agreed: good to comment why the offset is +ve or -ve (draw above / right, or draw below/left --> latter implies a negative offset).
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.
@vaughnbetz No problem. I have added comments about why the positive/negative offset is needed
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
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. Only had one suggestion for a comment (same place Keith flagged).
|
||
float draw_pin_offset; | ||
if (pin_rr.side() == TOP || pin_rr.side() == RIGHT) { |
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.
Agreed: good to comment why the offset is +ve or -ve (draw above / right, or draw below/left --> latter implies a negative offset).
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. |
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):

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)

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

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

Before GUI patching (M9K in Titan Architecture):

After GUI patching (M9K in Titan Architecture):

Before GUI patching (DSP36 in Titan Architecture):

After GUI patching (DSP36 in Titan Architecture):

**Note that there is no difference on normal architectures.
Before GUI patching (k6_N10_mem32K_40nm.xml):

After GUI patching (k6_N10_mem32K_40nm.xml):

The changes will only affect GUI codes under
vpr/src/draw
.Types of changes
Checklist: