Scatter-gather patterns for 3D architectures #3276
Conversation
…e function that it calls
soheilshahrouz
changed the title
There was a problem hiding this comment.
Gave a quick review.
@soheilshahrouz I love the cleanups you do, but man you got to split them across PRs; there's so much to review!
| // TODO: handle CHANZ nodes | ||
|
|
There was a problem hiding this comment.
TODO remaining in final code
|
|
||
| int from_layer_num = rr_graph.node_layer(from_node); | ||
| int to_layer_num = rr_graph.node_layer(to_node); | ||
| // TODO: handle CHANZ nodes that span multiple layers |
There was a problem hiding this comment.
TODO again. Not saying that you have to fix these right now, but a reminder that it is there. I sometimes forget about my TODOs so I'm just making sure that it's intended.
| const auto& grid = device_ctx.grid; | ||
|
|
||
| const size_t num_layers = grid.get_num_layers(); | ||
| const size_t chan_type_dim_size = (num_layers == 1) ? 2 : 3; |
There was a problem hiding this comment.
Add comment explaining this.
| if (is_chanxy(from_type) || is_chanz(from_type)) { | ||
| int from_layer_num = rr_graph.node_layer(from_node); | ||
| int to_layer_num = rr_graph.node_layer(to_node); | ||
| // TODO: handle CHANZ nodes |
There was a problem hiding this comment.
TODO. Again, just reminding that it exists.
| const t_chan_details& chan_details_x, | ||
| const t_chan_details& chan_details_y, | ||
| std::vector<t_chan_loc>& correct_channels) { | ||
| correct_channels.clear(); |
There was a problem hiding this comment.
If you're clearing this vector at the start of the function, why not just have it as return value then?
| t_physical_tile_loc chan_loc; | ||
| e_rr_type chan_type; | ||
|
|
||
| index_into_correct_chan(loc, side, chan_details_x, chan_details_y, chan_loc, chan_type); |
There was a problem hiding this comment.
The name for these two functions are way to similar, I thought it was recursive at first glance. I think 'index_to_correct_channels' should have sg or scatter_gather somewhere in the name to differentiate it better.
| for (const t_wire_switchpoints& wire_switchpoints : wire_switchpoints_vec) { | ||
| auto wire_type = vtr::string_view(wire_switchpoints.segment_name); | ||
|
|
||
| if (wire_type_sizes.find(wire_type) == wire_type_sizes.end()) { |
There was a problem hiding this comment.
Prefer using contains from c++20
| const t_chan_seg_details* chan_details = (chan_type == e_rr_type::CHANX) ? chan_details_x[chan_loc.x][chan_loc.y].data() : chan_details_y[chan_loc.x][chan_loc.y].data(); | ||
|
|
||
| for (const t_wire_switchpoints& wire_switchpoints : wire_switchpoints_vec) { | ||
| auto wire_type = vtr::string_view(wire_switchpoints.segment_name); |
There was a problem hiding this comment.
Why do we have a vtr::string_view? Maybe it's from when the project was not using C++17? Anyway I don't see why we need to use it now. Also, auto.
| int wire_switchpoint = get_switchpoint_of_wire(chan_type, chan_details[iwire], seg_coord, chan_side); | ||
|
|
||
| // Check if this wire belongs to one of the specified switchpoints; add it to our 'wires' vector if so | ||
| if (wire_switchpoint != valid_switchpoint) continue; |
There was a problem hiding this comment.
The loop seems backwards to me. You're going through all valid switchpoints then all wires, meaning that you do all the processing for the wire only to throw it away because in the outer loop we were looking for switchpoint number 1 instead of 2, even though switchpoint 2 might also be valid in the next iteration of the outer loop.
| index_to_correct_channels(sg_pattern.scatter_pattern, scatter_loc, chan_details_x, chan_details_y, scatter_channels); | ||
|
|
||
| if (gather_channels.empty() || scatter_channels.empty()) { | ||
| continue; |
There was a problem hiding this comment.
We should probably at least warn the user here that there's an ill formed pattern.
| auto gather_wire_candidates = find_candidate_wires(gather_channels, | ||
| sg_pattern.gather_pattern.from_switchpoint_set, | ||
| chan_details_x, chan_details_y, | ||
| wire_type_sizes_x, wire_type_sizes_y, | ||
| /*is_dest=*/false); | ||
|
|
||
| auto scatter_wire_candidates = find_candidate_wires(scatter_channels, | ||
| sg_pattern.scatter_pattern.to_switchpoint_set, | ||
| chan_details_x, chan_details_y, | ||
| wire_type_sizes_x, wire_type_sizes_y, | ||
| /*is_dest=*/true); |
| bottleneck_fanout = std::min<int>(bottleneck_fanout, scatter_wire_candidates.size()); | ||
|
|
||
| if (bottleneck_fanin == 0 || bottleneck_fanout == 0) { | ||
| continue; |
There was a problem hiding this comment.
ditto here, maybe we should warn the user. There's a potential problem of printing way too much stuff that should be considered.
| t_wire_switchpoint wire_switchpoint; ///< Wire index and its valid switchpoint | ||
| }; | ||
|
|
||
| /// Represents a scatter/gather bottleneck connection between two locations. |
There was a problem hiding this comment.
I think we referred to this as the "scatter-gather node" in the docs. I don't have an issue with the name bottleneck but I think you should say in the comments that the bottleneck connection is the same thing as scatter-gather node in the docs.
| const float R_minW_pmos, | ||
| const int wire_to_arch_ipin_switch, | ||
| int* wire_to_rr_ipin_switch) { | ||
| RRSwitchId* wire_to_rr_ipin_switch) { |
There was a problem hiding this comment.
Can you pass this by reference instead?
This PR includes the following changes: