RRGraphView node_type_string(), node_side_string() Implementation #1873
Conversation
RRGraphView Signed-off-by: Ethan Rogers <[email protected]>
Signed-off-by: Ethan Rogers <[email protected]>
ethanroj23
changed the title
Signed-off-by: Ethan Rogers <[email protected]>
|
Link to #1868 So that we keep tracking. |
There was a problem hiding this comment.
@ethanroj23 Look good to me. See if you want to do some optional refactoring here. In general, we can merge once CI is green. The removal on global variables can be done in later PRs by @baberali-rs and Rapidsilicon team
@vaughnbetz Let me know what you think. I may miss something.
| @@ -85,11 +85,11 @@ void report_overused_nodes() { | |||
| os << "Overused RR node #" << inode << '\n'; | |||
| os << "Node id = " << size_t(node_id) << '\n'; | |||
| os << "Occupancy = " << route_ctx.rr_node_route_inf[size_t(node_id)].occ() << '\n'; | |||
| os << "Capacity = " << device_ctx.rr_nodes.node_capacity(node_id) << "\n\n"; | |||
| os << "Capacity = " << rr_graph.node_capacity(node_id) << "\n\n"; | |||
There was a problem hiding this comment.
Feel good to see this function longer uses the rr_nodes data structure, it would be good to add the rr_graph to the argument list rather than fetching it from device_ctx. If it is too much for you, I can let @baberali-rs to follow up on this function.
There was a problem hiding this comment.
If we add const RRGraphView& rr_graph to the argument list, every function that calls report_overused_nodes() has to have a reference to the rr_graph. Currently, the call tree is like: main()->vpr_flow()->vpr_route_flow()->report_overused_nodes() How high up in the call tree do we want to go before we access the rr_graph from device_ctx? @vaughnbetz Do you have a preference on what to do here?
Additionally, if we pass the rr_graph all the way down from main(), each of the functions along the call tree will have rr_graph as an extra argument
There was a problem hiding this comment.
@tangxifan I have added RRGraphView to the argument list of report_overused_nodes() and now VTR Nightly Tests Part 1 is failing qor. I will try running the full qor regression, but it's possible we may have to leave things how they were. What do you think?
There was a problem hiding this comment.
@tangxifan now that this branch is passing, are we good to merge it in? I don't have write access, so I can't do that myself.
There was a problem hiding this comment.
Yes. I merge now. All concerns resolved.
| @@ -692,7 +692,7 @@ void print_route_tree(const t_rt_node* rt_node, int depth) { | |||
| auto& device_ctx = g_vpr_ctx.device(); | |||
| const auto& rr_graph = device_ctx.rr_graph; | |||
| VTR_LOG("%srt_node: %d (%s) \t ipin: %d \t R: %g \t C: %g \t delay: %g", | |||
| indent.c_str(), rt_node->inode, device_ctx.rr_nodes[rt_node->inode].type_string(), rt_node->net_pin_index, rt_node->R_upstream, rt_node->C_downstream, rt_node->Tdel); | |||
| indent.c_str(), rt_node->inode, rr_graph.node_type_string(RRNodeId(rt_node->inode)), rt_node->net_pin_index, rt_node->R_upstream, rt_node->C_downstream, rt_node->Tdel); | |||
There was a problem hiding this comment.
Similar to my last comment, it would be good to add the rr_graph to the argument list rather than fetching it from device_ctx. If it is too much for you, I can let @baberali-rs to follow up on this function.
an argument and removed the use of device_ctx within report_overused_nodes(). Signed-off-by: Ethan Rogers <[email protected]>
Description
In this PR, I have implemented
RRGraphView::node_type_string(node)andRRGraphView::node_side_string(node)throughout VTR. In order to do this, I followed a pattern similar to that in the last PR.Motivation and Context
These changes are a continuation of the RRGraphView API implementation effort.
How Has This Been Tested?
I have run the regression tests for
vtr_reg_strongalong with the QoR testing found at$ ../scripts/run_vtr_task.py regression_tests/vtr_reg_nightly_test3/vtr_reg_qor_chain. Results from these QoR tests can be found below. The file containing all results can be found here.Only rows that are different are shown. For some reason,
vtr_flow_elapsed_timewas empty in the spreadsheet.Types of changes
Checklist: