Flat Routing Visualization #3159
Conversation
github-actions
bot
added
VPR
SamuelHo10
force-pushed
the
visualize_flat_routing
branch
from
4410643 to
571e50d
Compare
|
Thank you for your changes! Could you please share some screenshots showing how the intra-logic connections are drawn? Also, it would be helpful if you could include a screenshot for the case when the flat-router is not enabled, just to ensure the default flow remains intact. I’m currently testing this PR locally, but having the screenshots here would be useful for future reference. |
|
For Future Reference, Flat Routing Enabled: Flat Routing Disabled |
There was a problem hiding this comment.
Looks good; some changes requested though.
As we discussed in the meeting, you should:
- fix the arrow direction (done I think)
- test more architecture files.
- Stratix IV: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/arch/titan/stratixiv_arch.timing.xml (need to use with a titan benchmark; neuron is the smallest normal one, but titan_other has some smaller ones still)
- Stratix 10: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/arch/titan/stratix10_arch.timing.xml (need to use a Stratix 10 mapped Titan design or a Titanium design)
- Artix 7: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/arch/xilinx/7series_BRAM_DSP_carry.xml
- Z1000: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/arch/zeroasic/z1000/z1000.xml
- The arch we use with Koios (hopefully the developer guide points you at the right task)
| @@ -27,6 +27,7 @@ void draw_rr_edges(RRNodeId from_node, ezgl::renderer* g); | |||
|
|
|||
| void draw_rr_chan(RRNodeId inode, const ezgl::color color, ezgl::renderer* g); | |||
|
|
|||
| void draw_rr_intrapin(RRNodeId inode, const ezgl::color& color, ezgl::renderer* g); | |||
There was a problem hiding this comment.
Add a comment describing what the function does. I think it would be best to make it a Doxygen comment so we can choose to build Doxygen comments for more drawing code in the future.
General principle: comment what a function does in the .h file, or (if it is a static function) in its declaration in the .cpp file. Use Doxygen formatting in either case.
| int height = (int)device_ctx.grid.height(); | ||
| if (chanx_track.empty()) { | ||
| chanx_track = vtr::OffsetMatrix<int>({{{1, width - 1}, {0, height - 1}}}); | ||
| // Draw Pins |
There was a problem hiding this comment.
Isn't this drawing rr nodes (not just pins)? Suggest changing the comment.
| for (int i = 1; i < width - 1; i++) | ||
| for (int j = 0; j < height - 1; j++) | ||
| chanx_track[i][j] = (-1); | ||
| // Draw IO Pins |
There was a problem hiding this comment.
Draw cluster-level IO pins
| continue; | ||
| } | ||
|
|
||
| auto iedge = find_edge(prev_node, inode); | ||
| auto switch_type = rr_graph.edge_switch(RRNodeId(prev_node), iedge); | ||
| // Skip drawing sources and sinks |
There was a problem hiding this comment.
Skip drawing edges to or from sources and sinks
| } else { | ||
| draw_pin_to_chan_edge(inode, prev_node, g); | ||
| } | ||
| if (rr_graph.node_type(prev_node) == e_rr_type::OPIN) { |
There was a problem hiding this comment.
To match the structure of the rest of the code and shorten this routine I think this switch statement should be moved into a helper router (maybe called draw_inter_cluster_rr_edge )
| @@ -400,7 +416,7 @@ static void draw_internal_pb(const ClusterBlockId clb_index, t_pb* pb, const ezg | |||
| if (draw_state->draw_block_text) { | |||
| g->draw_text( | |||
| ezgl::point2d(abs_bbox.center_x(), | |||
| abs_bbox.top() - (abs_bbox.height()) / 15.0), | |||
| abs_bbox.top() - draw_coords->get_tile_height() * 0.01), | |||
There was a problem hiding this comment.
Suggest tying this 0.01 to one of the const drawing values above, or making a new one. It'll be hard to find and update/align this 0.01 with other drawing offsets in future changes if you don't give it a name (or even better, compute it from the constants already present).
| @@ -858,13 +858,13 @@ t_pb_graph_pin* get_pb_graph_node_pin_from_model_port_pin(const t_model_ports* m | |||
| } | |||
|
|
|||
| //Retrieves the atom pin associated with a specific CLB and pb_graph_pin | |||
| // Warning: Not all pb_graph_pins are associated with an atom pin! | |||
There was a problem hiding this comment.
I'd expand on this. Some represents nodes/pins within the pb_graph hierarchy, but not on a primitive.
|
|
||
| VTR_ASSERT(pb_graph_pin); | ||
|
|
||
| return {blk_id, pb_graph_pin}; |
There was a problem hiding this comment.
Need to very clearly comment this return type in the Doxygen comment in the .h file (use @return). Consider using a named structure. A pair could be OK with enough commenting. If you stick with a pair, callers should extract the data into named variables for clarity, or (even better) use something like:
auto [blk_id, pb_graph_pin] = get_rr_node_cluster_blk_id_pb_graph_pin ( ...)
| /** | ||
| * @brief Returns the atom pin ID for the given RR node ID. | ||
| * **Warning**: This function should be called only if flat-router is enabled, | ||
| * since, otherwise, the routing resources inside clusters are not added to the RR graph. Also, not all RRNodes have an AtomPinId associated with them. |
There was a problem hiding this comment.
What do you do if the rr_node_id does not correspond to an atom pin? Return an invalid ID? assert? (Bad: silently corrupt memory or you don't know!).
Comment this ... OK to either return the invalid ID if this isn't an atom_pin, or OK to assert but you need to comment which. I lean toward returning the invalid id in that case since it might be useful for some if statements in the future, but your call.
| ClusterBlockId blk_id; | ||
| t_pb_graph_pin* pb_graph_pin; | ||
| std::tie(blk_id, pb_graph_pin) = get_rr_node_cluster_blk_id_pb_graph_pin(rr_node_id); | ||
|
|
There was a problem hiding this comment.
Should have some VTR_ASSERT_SAFE calls here to verify that this rr_node_id does in fact correspond to an atom pin.
VTR_ASSERT_SAFE is turned off in release build; you could also use VTR_ASSERT if this code isn't very hot and you always want it checked.
Several changes were made for flat routing visualization.
Firstly, I modified the draw intra-logic block code so that the margins between blocks are absolute and not based on relative size. The intra-logic blocks are also now drawn to be not long and skinny, and instead must have a reasonable width-to-height ratio. Furthermore, the intra-logic blocks of maximum depth were previously not drawn. That has also been fixed.
To get the pin locations of intracluster RRNodeId, I made helper functions to convert RRNodeId into cluster_blk_id and pb_graph_pin. I also separated the code which draws edges and pins, primarily for readability and also to avoid repetition. The loops with clusternets were replaced with atomnets when flat routing is enabled. Lastly, for testing and debugging purposes, the ability to click on intracluster pins to highlight the net was added.