Cleaned up RRGraph Drawing Code #3206
Conversation
github-actions
bot
added
VPR
|
It's probably good to add SOURCE and SINK viewing, probably with another checkbox to control clutter. You'd probably draw sources and sinks in some visually distinct way (circle for one and ellipse for the other?) with edges to pins where they connect. Physically they are inside the block, so in the interior of cluster-level blocks makes sense. For primitive level they would ideally be drawn in the primitive they belong to. But you'll have to figure out which primitive that is ... the net_rr_terminals data structure loader also figures that out so you could look at it. |
There was a problem hiding this comment.
Nice cleanup!
Some comments embedded.
Biggest one: check the highlighting of rr_nodes works properly; you can simplify the code or change how it works, but we should be able to highlight nodes by (right) clicking on them and we should see their fanin and fanout edges highlighted too.
Future-proofing / next: @amin1377 has added support for a new MUX type. After this PR is in, you should test with an openFPGA tileable rr-graph on a small device to ensure it draws properly, and you should add MUX support. MUXes are like 0-length wires; we can draw them as points in a switch block I think but we'll have to allocate space for them etc. so it is probably a reasonable amount of work.
Future proofing 2: @soheilshahrouz has added support for CHANZ nodes. You should make sure those draw too in a future PR. At least we shouldn't crash, and most code that checks for CHANX and CHANY should have a CHANZ case too.
| from_type = rr_graph.node_type(rr_node); | ||
| from_type = rr_graph.node_type(inode); | ||
| if (from_type == e_rr_type::SOURCE || from_type == e_rr_type::SINK || !is_inter_cluster_node(rr_graph, inode)) { | ||
| return; |
There was a problem hiding this comment.
Add a short comment
// Currently don't visualize sources or sinks
| to_type = rr_graph.node_type(to_node); | ||
| bool edge_configurable = rr_graph.edge_is_configurable(inode, iedge); | ||
|
|
||
| if (!is_edge_valid_to_draw(RRNodeId(to_node), rr_node)) | ||
| continue; // skip drawing if edge is not valid to draw | ||
| if (to_type == e_rr_type::SOURCE || to_type == e_rr_type::SINK || !is_inter_cluster_node(rr_graph, to_node)) { |
There was a problem hiding this comment.
Add a short comment
// Currently don't visualize sources or sinks
| "in draw_rr_edges: node %d (type: %d) connects to node %d (type: %d).\n", | ||
| inode, from_type, to_node, to_type); | ||
| break; | ||
| if (to_type == e_rr_type::CHANX || to_type == e_rr_type::CHANY) { |
There was a problem hiding this comment.
Should add type MUX to this (Amin just added it). Can use the same colour as if it were a CHANX or CHANY.
| if (to_type == e_rr_type::IPIN) { | ||
| color = blk_LIGHTSKYBLUE; | ||
| if (draw_state->draw_rr_node[to_node].node_highlighted && draw_state->draw_rr_node[inode].color == DEFAULT_RR_NODE_COLOR) { | ||
| // If the IPIN is clicked on, draw connection to all the CHANX |
There was a problem hiding this comment.
I think the overall highlighting code needs to be tested and this comment needs to be hoisted higher and updated.
I believe you can right click on an rr-node, and its fanin and fanout will be highlighted in certain colours. The highlighted node itself is drawn in magenta. You should test exactly what happens now, and see if you like it. I think the code can be simplified (which you've done) but you need to test we're doing some reasonable highlighting (idea: right click highlights and rr-node and its fan-in edges and fan-out edges).
There was a problem hiding this comment.
Maybe the highlighting can be simplified to something that just looks at the state of the prior and current rr-nodes (without paying attention to their type) and decides if an edge should be highlighted or not. I suspect we're being overly complex with the code that checks the node types (CHANX etc.). If we can simplify, we could just move it to a helper function to find the colour.
| prev_coord = {inter_cluster_x, inter_cluster_y}; | ||
| icoord = intra_cluster_coord; | ||
| } else { | ||
| VPR_ERROR(VPR_ERROR_DRAW, "Invalid pin edge direction: %d", pin_edge_dir); |
There was a problem hiding this comment.
Can I have edges that connect two INTRA_CLUSTER pins? I believe we can at intermediate hierarchy levels in a clustered block / pb_type. You should update the code to handle that case if I'm right.
| // determine the location of the pins | ||
| float inter_cluster_x, inter_cluster_y; | ||
| ezgl::point2d intra_cluster_coord; | ||
| for (const e_side& pin_side : TOTAL_2D_SIDES) { |
There was a problem hiding this comment.
side is char or int
don't need to iterate over it by reference
| bool inode_inter_cluster = is_inter_cluster_node(rr_graph, inode); | ||
| int current_node_layer = rr_graph.node_layer(inode); | ||
|
|
||
| e_rr_type prev_type = rr_graph.node_type(RRNodeId(prev_node)); |
There was a problem hiding this comment.
Why is prev_node casted here?
| void draw_inter_cluster_rr_edge(RRNodeId inode, RRNodeId prev_node, e_rr_type rr_type, e_rr_type prev_type, ezgl::renderer* g) { | ||
| const RRGraphView& rr_graph = g_vpr_ctx.device().rr_graph; | ||
| t_edge_size iedge = find_edge(prev_node, inode); | ||
| short switch_type = rr_graph.edge_switch(RRNodeId(prev_node), iedge); |
There was a problem hiding this comment.
is casting prev_node required?
| short switch_type = rr_graph.edge_switch(RRNodeId(prev_node), iedge); | ||
|
|
||
| switch (rr_type) { | ||
| case e_rr_type::IPIN: { |
There was a problem hiding this comment.
I think braces are not necessary for case statements.
Everything that comes after case is executed no matter they are surrounded by braces or not.
|
|
||
| from_type = rr_graph.node_type(rr_node); | ||
| from_type = rr_graph.node_type(inode); |
There was a problem hiding this comment.
It is usually a good idea to have the declaration and definition in the same line, if possible.
Edges for each side are now drawn between inter-cluster and intra-cluster nodes. I’ve simplified the RRGraph drawing code by introducing reusable helper functions draw_rr_edge and draw_rr_node, while preserving all existing functionality and color schemes. I’ve also disabled the code that was drawing sources and sinks, as it didn’t seem to render correctly to begin with. Would it make sense to reimplement source/sink visualization, or is it something we want to leave out for now? In the next PR, I will probably hook up the new buttons to RRGraph drawing, so I left the toggle_RR variable unchanged for now.