Add tileable RR Graph #3134
Add tileable RR Graph #3134
Conversation
github-actions
bot
added
VPR
|
@soheilshahrouz: Thank you for taking the time to review this PR. I’ve addressed all your comments, and I think this PR is now ready for the next round of review. |
There was a problem hiding this comment.
Done reviewing up to the start of rr_spatial_lookup (more to do).
Lots of work here, thanks Amin!
Big picture comments:
Memory: don't allocate things we don't need to (bends, ptc_num array). Instead, allocate only if tileable is on, and add checking to the get/set functions for these.
Clarity: ptc_nums is confusing. Need to give it a different name to avoid confusion (can't have two ptc_nums). I suggest tileable_track_num or something like that.
bend_start / bend_end are mysterious. Need to completely explain what they are returning.
Various other comments in the text.
QoR: looks mostly good, but Titan routing slowed down 3%. May be due to the memory footprint --> making the allocation changes above will hopefully fix.
Longer term: doesn't have to be in this PR, but right afterwards we should get rid of the tiling data we added earlier to the RRgraph in an attempt to unify a bit of OpenFPGA and VTR code. Clearly we won't use that, so storing something of O(rr_nodes) is not a good idea when no one uses it.
…rilog-to-routing into add_tileable_rr_graph
|
Thanks @vaughnbetz, @soheilshahrouz, and @AlexandreSinger for your reviews. I’ve addressed your comments, and I believe the PR is now ready for the next round of review if you’d like. |
…meters since it relies on that value
…rilog-to-routing into add_tileable_rr_graph
| int wire_to_rr_ipin_switch, | ||
| enum e_base_cost_type base_cost_type); | ||
|
|
||
| std::vector<t_clb_to_clb_directs> alloc_and_load_clb_to_clb_directs(const std::vector<t_direct_inf>& directs, const int delayless_switch); |
There was a problem hiding this comment.
this function naturally belongs to clb_2clb_directs.h
rr_graph.cpp is too large
moving the implementation of this function to a newly created clb2clb_directs.cpp can make the code more organized
| det_routing_arch.through_channel, /* Allow/Prohibit through tracks across multi-height and multi-width grids */ | ||
| det_routing_arch.opin2all_sides, /* Allow opin of grid to directly drive routing tracks at all sides of a switch block */ | ||
| det_routing_arch.concat_wire, /* Allow end-point tracks to be wired to a starting point track on the opposite in a switch block. It means a wire can be continued in the same direction to another wire */ | ||
| det_routing_arch.concat_pass_wire, /* Allow passing tracks to be wired to the routing tracks in the same direction in a switch block. It means that a pass wire can jump in the same direction to another */ |
There was a problem hiding this comment.
/* -> //
several instances in this file
| is_flat, | ||
| Warnings, | ||
| router_opts.route_verbosity); | ||
| if (e_graph_type::UNIDIR_TILEABLE != graph_type) { |
There was a problem hiding this comment.
now that we have two separate rr graph generators, I think we should this function to another file and keep the source code of these two generators as separate as we can
| const enum e_directionality directionality, | ||
| bool* Fc_clipped, | ||
| bool is_flat) { | ||
| //Initialize Fc of all blocks to zero |
There was a problem hiding this comment.
bring back the space after //
| @@ -1227,8 +1227,8 @@ static void run_graphics_commands(const std::string& commands) { | |||
| t_draw_state backup_draw_state = *draw_state; | |||
|
|
|||
| std::vector<std::vector<std::string>> cmds; | |||
| for (std::string raw_cmd : vtr::split(commands, ";")) { | |||
| cmds.push_back(vtr::split(raw_cmd)); | |||
| for (std::string raw_cmd : vtr::StringToken(commands).split(";")) { | |||
There was a problem hiding this comment.
Is this copy necessary?
| ************************************************************************/ | ||
| short find_rr_graph_min_fan_in(const RRGraph& rr_graph, | ||
| const std::vector<e_rr_type>& node_types) { | ||
| short min_fan_in = 0; |
There was a problem hiding this comment.
shouldn't this be initilaized with std::numeric_limits::max()?
| const std::vector<e_rr_type>& node_types) { | ||
| short min_fan_in = 0; | ||
|
|
||
| for (const RRNodeId& node : rr_graph.nodes()) { |
There was a problem hiding this comment.
don't need to iterate by ref
| const int& pin_height) { | ||
| std::vector<int> pin_list; | ||
| /* Make sure a clear start */ | ||
| pin_list.clear(); |
|
|
||
| /* For IO_TYPE sides */ | ||
| t_physical_tile_type_ptr phy_tile_type = grids.get_physical_type(t_physical_tile_loc(x, y, layer)); | ||
| for (const e_side& side : io_side) { |
There was a problem hiding this comment.
don't iterate by ref
| } | ||
|
|
||
| /************************************************************************ | ||
| * Idenfity if a X-direction routing channel exist in the fabric |
Merging OpenFPGA branch into master branch. PR #2135 explains features of OpenFPGA.